[cmake-guidelines] Minor update, for `if()` #18980

strega-nil · 2021-07-16T22:23:32Z

Basically, introduces rules for if() as well as argument quoting.

Depends on #18397

Neumann-A · 2021-07-16T22:31:54Z

docs/maintainers/cmake-guidelines.md

+  - Exception: arguments to `if()` that are not operators must always be quoted.
+    - operators should be unquoted.
+    - Example:
+      ```cmake
+      if("${FOO}" STREQUAL "BAR")
+      endif()
+
+      if("${BAZ}" EQUAL "0")
+      endif()
+      ```


is there a reason to not use if(FOO STREQUAL "BAR") ? If FOO is not set this will raise and error.

no it won't, in either case.

Assuming FOO is not defined:

if(FOO STREQUAL "BAR") # equivalent to `if("FOO" STREQUAL "BAR")` endif() if("${FOO}" STREQUAL "BAR") # equivalent to `if("" STREQUAL "BAR")` endif()

if(FOO STREQUAL "FOO") message(STATUS "yes 1") endif() if("${FOO}" STREQUAL "") message(STATUS "yes 2") endif()

prints out

-- yes 1 -- yes 2

But FOO STREQUAL "BAR" does have exactly the desired semantics in this case. I don't think this rule is needed, since we already have rules saying all referenced variables should be previously defined:

Variables are not assumed to be empty.
If the variable is intended to be used locally,
it must be explicitly initialized to empty with set(foo "") if it is a string variable,
and vcpkg_list(SET foo) if it is a list variable.

Ok after testing
if(${FOO} STREQUAL "BAR")
is throwing the error I thought about.

I wonder if
if(FOO AND FOO STREQUAL "BAR")
is measurable more performant than
if("${FOO}" STREQUAL "BAR")

docs/maintainers/cmake-guidelines.md

ras0219-msft · 2021-07-16T22:50:09Z

docs/maintainers/cmake-guidelines.md

+  - Exception: arguments to `if()` that are not operators must always be quoted.
+    - operators should be unquoted.
+    - Example:
+      ```cmake
+      if("${FOO}" STREQUAL "BAR")
+      endif()
+
+      if("${BAZ}" EQUAL "0")
+      endif()
+      ```


But FOO STREQUAL "BAR" does have exactly the desired semantics in this case. I don't think this rule is needed, since we already have rules saying all referenced variables should be previously defined:

Variables are not assumed to be empty.
If the variable is intended to be used locally,
it must be explicitly initialized to empty with set(foo "") if it is a string variable,
and vcpkg_list(SET foo) if it is a list variable.

docs/maintainers/cmake-guidelines.md

strega-nil · 2021-07-16T23:18:22Z

The reason I'm interested in this change is not because if(FOO STREQUAL "bar") can't be used without error, it's because it's extremely confusing as to when it can be used -- for example, I've had bugs in the past where I've written if(EXISTS arg_FILENAME) instead of the required if(EXISTS "${arg_FILENAME}"), or if("${foo}" MATCHES regex_var).

strega-nil-ms · 2021-07-26T20:46:29Z

Closed for rollup

[cmake-guidelines] Minor update, for `if()`

@strega-nil

* [rollup:2021-07-26 1/6] PR #18783 (@strega-nil) [scripts-audit] vcpkg_copy_tools and friends * [rollup:2021-07-26 2/6] PR #18898 (@dg0yt) [vcpkg] Fix toolchain compatibility with cmake < 3.15 * [rollup:2021-07-26 3/6] PR #18980 (@strega-nil) [cmake-guidelines] Minor update, for `if()` * [rollup:2021-07-26 4/6] PR #18981 (@strega-nil) [scripts-audit] vcpkg_check_linkage * [rollup:2021-07-26 5/6] PR #19158 (@Hoikas) [vcpkg.cmake] Fix variable case. * [rollup:2021-07-26 6/6] PR #18839 [scripts-audit] z_vcpkg_get_cmake_vars Co-authored-by: nicole mazzuca <mazzucan@outlook.com>

new guidelines for if and also for what kinds of args to use

0cf1ec8

strega-nil-ms force-pushed the cmake-guidelines-if branch from fe25710 to 0cf1ec8 Compare July 16, 2021 22:25

Neumann-A reviewed Jul 16, 2021

View reviewed changes

update for vcpkg_list

c98deff

ras0219-msft reviewed Jul 16, 2021

View reviewed changes

Neumann-A reviewed Jul 16, 2021

View reviewed changes

docs/maintainers/cmake-guidelines.md Show resolved Hide resolved

strega-nil-ms force-pushed the cmake-guidelines-if branch from 22567d3 to a928b6c Compare July 16, 2021 23:15

slightly less strict rules for if

127e982

strega-nil-ms force-pushed the cmake-guidelines-if branch from a928b6c to 127e982 Compare July 17, 2021 00:43

strega-nil-ms added category:scripts-audit Part of the scripts audit effort. info:internal This PR or Issue was filed by the vcpkg team. labels Jul 17, 2021

JonLiu1993 assigned PhoebeHui Jul 19, 2021

strega-nil-ms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jul 21, 2021

booleans should be unquoted

4097a0b

strega-nil-ms added requires:discussion and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Jul 22, 2021

meeting

0f5da5d

strega-nil-ms mentioned this pull request Jul 26, 2021

[rollup] 2021-07-26 #19157

Merged

strega-nil-ms closed this Jul 26, 2021

strega-nil-ms pushed a commit to strega-nil/vcpkg that referenced this pull request Jul 28, 2021

[rollup:2021-07-26 3/6] PR microsoft#18980 (@strega-nil)

81fdca6

[cmake-guidelines] Minor update, for `if()`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cmake-guidelines] Minor update, for `if()` #18980

[cmake-guidelines] Minor update, for `if()` #18980

strega-nil commented Jul 16, 2021 •

edited

Loading

Neumann-A Jul 16, 2021

strega-nil Jul 16, 2021

strega-nil Jul 16, 2021

ras0219-msft Jul 16, 2021

Neumann-A Jul 16, 2021 •

edited

Loading

ras0219-msft Jul 16, 2021

strega-nil commented Jul 16, 2021

strega-nil-ms commented Jul 26, 2021

[cmake-guidelines] Minor update, for if() #18980

[cmake-guidelines] Minor update, for if() #18980

Conversation

strega-nil commented Jul 16, 2021 • edited Loading

Neumann-A Jul 16, 2021

Choose a reason for hiding this comment

strega-nil Jul 16, 2021

Choose a reason for hiding this comment

strega-nil Jul 16, 2021

Choose a reason for hiding this comment

ras0219-msft Jul 16, 2021

Choose a reason for hiding this comment

Neumann-A Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

ras0219-msft Jul 16, 2021

Choose a reason for hiding this comment

strega-nil commented Jul 16, 2021

strega-nil-ms commented Jul 26, 2021

[cmake-guidelines] Minor update, for `if()` #18980

[cmake-guidelines] Minor update, for `if()` #18980

strega-nil commented Jul 16, 2021 •

edited

Loading

Neumann-A Jul 16, 2021 •

edited

Loading