Deduplicate string literals. #1355

BillyONeal · 2024-02-27T02:55:11Z

In discussion with @ras0219-msft, Robert argues that the purpose of these constants being separately declared variables is to avoid typos. By that standard, they should not be declared wherever they are used, they should be declared in one place and reused. This change does that.

See also #1329 (comment)

Note that the json constants in the recursive environment blob were renamed to be consistent with other JSON identifiers, but because this is our exe communicating with the same exe this is OK.

…hese constants being separately declared variables is to avoid typos. By that standard, they should not be declared wherever they are used, they should be declared in one place and reused. This change does that. See also microsoft#1329 (comment) Note that the json constants in the recursive environment blob were renamed to be consistent with other JSON identifiers, but because this is our exe communicating with the same exe this is OK.

ras0219-msft

(Through commands.add.cpp and From commands.z-extract.cpp)

ras0219-msft · 2024-02-29T00:34:13Z

src/vcpkg/vcpkgcmdarguments.cpp

            StabilityTag::Standard,
            args.host_triplet,
            msg::format(msgSpecifyHostArch, msg::env_var = format_environment_variable("VCPKG_DEFAULT_HOST_TRIPLET")));
-        args.parser.parse_option(MANIFEST_ROOT_DIR_ARG, StabilityTag::Experimental, args.manifest_root_dir);
-        args.parser.parse_option(BUILDTREES_ROOT_DIR_ARG,
+        args.parser.parse_option(JsonIdManifestRoot, StabilityTag::Experimental, args.manifest_root_dir);


Suggested change

args.parser.parse_option(JsonIdManifestRoot, StabilityTag::Experimental, args.manifest_root_dir);

args.parser.parse_option(SwitchManifestRoot, StabilityTag::Experimental, args.manifest_root_dir);

ras0219-msft · 2024-02-29T01:00:54Z

src/vcpkg/configuration.cpp

-            RegistryConfigDeserializer::NAME,
-            RegistryConfigDeserializer::LOCATION,
-        };
+            JsonIdKind, JsonIdBaseline, JsonIdPath, JsonIdRepository, JsonIdReference, JsonIdName, JsonIdLocation};


Nit suggestion: trailing comma.

ras0219-msft · 2024-02-29T01:03:30Z

src/vcpkg/configuration.cpp


-        if (kind == KIND_BUILTIN)
+        if (kind == JsonIdBuiltin)


Consideration: should these semantically be JsonValueBuiltin instead, since they aren't keys? (As-is also LGTM)

I originally had JsonFieldXxx but this convinced me to do JsonId instead because they were sometimes values.

ras0219-msft · 2024-02-29T18:58:42Z

src/vcpkg/commands.build.cpp

        };

        const std::string empty;
        for (auto&& kv : VCPKG_OPTIONS)
        {
-            const std::string& variable_value = Util::value_or_default(cmakevars, kv.first, empty);
+            const auto& variable_value = Util::value_or_default(cmakevars, kv.first.to_string(), empty);


The lack of types makes me uncertain about temporary lifetime extension etc. Perhaps this can change to simply be StringLiterals?

Suggested change

const auto& variable_value = Util::value_or_default(cmakevars, kv.first.to_string(), empty);

StringLiteral variable_value = Util::value_or_default(cmakevars, kv.first, StringLiteral{});

This is not possible because cmakevars is const std::unordered_map<std::string, std::string>&, and unordered_map is not transparent. (If it were std::map<std::string, std::string, std::less<>> we could)

See also #1362

ras0219-msft · 2024-02-29T19:56:28Z

src/vcpkg/commands.build.cpp

@@ -1165,8 +1141,8 @@ namespace vcpkg
        const auto& triplet_abi = paths.get_triplet_info(pre_build_info, toolset);
        abi_info.triplet_abi.emplace(triplet_abi);
        const auto& triplet_canonical_name = action.spec.triplet().canonical_name();
-        abi_tag_entries.emplace_back("triplet", triplet_canonical_name);
-        abi_tag_entries.emplace_back("triplet_abi", triplet_abi);
+        abi_tag_entries.emplace_back(AbiTagIdTriplet, triplet_canonical_name);


Why are these AbiTagIdXYZ but below we have AbiTagCMake?

Dropped the id :)

ras0219-msft · 2024-02-29T20:00:41Z

src/vcpkg/commands.set-installed.cpp

-            detector.insert("name", Json::Value::string("vcpkg"));
-            detector.insert("url", Json::Value::string("https://github.com/microsoft/vcpkg"));
-            detector.insert("version", Json::Value::string("1.0.0"));
+            detector.insert(JsonIdName, Json::Value::string(JsonIdVcpkg));


JsonIdVcpkg in this case seems more like an "unrelated constant" like the URL and version below.

…terals # Conflicts: # src/vcpkg/commands.add-version.cpp # src/vcpkg/commands.set-installed.cpp # src/vcpkg/registries.cpp # src/vcpkg/sourceparagraph.cpp # src/vcpkg/versiondeserializers.cpp

…s0219-msft

…s0219-msft

--------------------------------------------- D:\vcpkg-tool\include\vcpkg\base\contractual-constants.h(438, 36) D:\vcpkg-tool\src\vcpkg\commands.build.cpp(1138, 38) Renamed 'AbiTagIdTripletAbi' to 'AbiTagTripletAbi': --------------------------------------------------- D:\vcpkg-tool\include\vcpkg\base\contractual-constants.h(439, 36) D:\vcpkg-tool\src\vcpkg\commands.build.cpp(1139, 38)

Followup to microsoft#1355 Also renames the JSON documents that talk to tests and to artifacts to follow the convention used elsewhere.

Thomas1664 · 2024-03-18T12:06:21Z

@BillyONeal IMO the possible benefit of avoiding typos isn't worth it: By making string literals a concrete type, they can't easily be fed into things expecting strings. One has to use .to_string() to make it work which adds clutter. Furthermore, this causes merge conflicts with every open PR. I would argue this actually causes more typos by picking the wrong string literal.

I think you should focus on more important tasks rather than making life harder for everyone contributing (assuming making life harder for everyone isn't an important task for you).

BillyONeal mentioned this pull request Feb 27, 2024

Accept arbitrary version formats in overrides. #1329

Merged

clang-format

c21a4b4

ras0219-msft reviewed Feb 29, 2024

View reviewed changes

ras0219-msft approved these changes Feb 29, 2024

View reviewed changes

BillyONeal added 5 commits March 1, 2024 16:58

Merge remote-tracking branch 'origin/main' into deduplicate-string-li…

a5bd0cb

…terals # Conflicts: # src/vcpkg/commands.add-version.cpp # src/vcpkg/commands.set-installed.cpp # src/vcpkg/registries.cpp # src/vcpkg/sourceparagraph.cpp # src/vcpkg/versiondeserializers.cpp

JsonIdManifestRoot => SwitchManifestRoot code reivew comment from @ra…

86d110b

…s0219-msft

Add trailing comma suggested by @ras0219-msft.

b7d7bdf

Remove JsonIdVcpkg as requested by @ras0219-msft

f5adb81

BillyONeal merged commit 05f88ed into microsoft:main Mar 2, 2024
5 checks passed

BillyONeal deleted the deduplicate-string-literals branch March 2, 2024 02:35

BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Mar 2, 2024

Deduplicate even more things

7435367

Followup to microsoft#1355 Also renames the JSON documents that talk to tests and to artifacts to follow the convention used elsewhere.

BillyONeal mentioned this pull request Mar 2, 2024

Deduplicate even more things #1359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate string literals. #1355

Deduplicate string literals. #1355

BillyONeal commented Feb 27, 2024

ras0219-msft left a comment

ras0219-msft Feb 29, 2024

ras0219-msft Feb 29, 2024

ras0219-msft Feb 29, 2024

BillyONeal Mar 2, 2024

ras0219-msft Feb 29, 2024

BillyONeal Mar 2, 2024

BillyONeal Mar 7, 2024

ras0219-msft Feb 29, 2024

BillyONeal Mar 2, 2024

ras0219-msft Feb 29, 2024

Thomas1664 commented Mar 18, 2024 •

edited

Loading

	args.parser.parse_option(JsonIdManifestRoot, StabilityTag::Experimental, args.manifest_root_dir);
	args.parser.parse_option(SwitchManifestRoot, StabilityTag::Experimental, args.manifest_root_dir);

	const auto& variable_value = Util::value_or_default(cmakevars, kv.first.to_string(), empty);
	StringLiteral variable_value = Util::value_or_default(cmakevars, kv.first, StringLiteral{});

Deduplicate string literals. #1355

Deduplicate string literals. #1355

Conversation

BillyONeal commented Feb 27, 2024

ras0219-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thomas1664 commented Mar 18, 2024 • edited Loading

Thomas1664 commented Mar 18, 2024 •

edited

Loading