[Clang] avoid relying on StringMap iteration order when roundtripping -analyzer-config

[erikdesjardins]

Summary

I am working on another patch that changes StringMap's hash function,
which changes the iteration order here, and breaks some tests,
specifically:

clang/test/Analysis/NSString.m
clang/test/Analysis/shallow-mode.m

with errors like:

generated arguments do not match in round-trip
generated arguments #1 in round-trip: <...> "-analyzer-config" "ipa=inlining" "-analyzer-config" "max-nodes=75000" <...>
generated arguments #2 in round-trip: <...> "-analyzer-config" "max-nodes=75000" "-analyzer-config" "ipa=inlining" <...>

To avoid this, sort the options by key, instead of using the default map
iteration order.

[jansvoboda11]

Thanks for the patch. Left a couple of small suggestions.

clang/lib/Frontend/CompilerInvocation.cpp

880–887

Per our coding style, we don't put braces around bodies with just a single statement.

881

I think you should be able to drop llvm::.

881

No reason to allocate for this temporary storage IMO, you could replace std::string with StringRef.

887

You could use C++17 structured bindings: for (const auto &[Key, Value] : SortedConfigOpts) { ... }.

[MaskRay]

clang/lib/Frontend/CompilerInvocation.cpp

884

If you use pair instead of tuple, you can use llvm::less_first()

[dblaikie]

clang/lib/Frontend/CompilerInvocation.cpp

880–887

We usually skip braces on single line blocks

881

Perhaps std::pair would be slightly easier to read than std::tuple?

887–889

maybe a structured binding here?

[dblaikie]

(heh, don't mind my feedback being duplicate - didn't refresh before submitting - glad other folks got to it before me! :) )

[erikdesjardins]

Improve code style. ninja check-clang still passes (as expected)

[erikdesjardins]

I don't have commit access, please commit this on my behalf: Erik Desjardins <[e-mail]>

clang/lib/Frontend/CompilerInvocation.cpp

for (const auto &CP : Opts.CheckersAndPackages) {
     OptSpecifier Opt =
         CP.second ? OPT_analyzer_checker : OPT_analyzer_disable_checker;
     GenerateArg(Args, Opt, CP.first, SA);
   }
 
   AnalyzerOptions ConfigOpts;
   parseAnalyzerConfigs(ConfigOpts, nullptr);
 
-  for (const auto &C : Opts.Config) {
+  // Sort options by key to avoid relying on StringMap iteration order.
+  SmallVector<std::pair<StringRef, StringRef>, 4> SortedConfigOpts;
+  for (const auto &C : Opts.Config)
+    SortedConfigOpts.emplace_back(C.getKey(), C.getValue());
+  llvm::sort(SortedConfigOpts, llvm::less_first());
+
+  for (const auto &[Key, Value] : SortedConfigOpts) {
     // Don't generate anything that came from parseAnalyzerConfigs. It would be
     // redundant and may not be valid on the command line.
-    auto Entry = ConfigOpts.Config.find(C.getKey());
-    if (Entry != ConfigOpts.Config.end() && Entry->getValue() == C.getValue())
+    auto Entry = ConfigOpts.Config.find(Key);
+    if (Entry != ConfigOpts.Config.end() && Entry->getValue() == Value)
       continue;
 
-    GenerateArg(Args, OPT_analyzer_config, C.getKey() + "=" + C.getValue(), SA);
+    GenerateArg(Args, OPT_analyzer_config, Key + "=" + Value, SA);
   }
 
   // Nothing to generate for FullCompilerInvocation.
 }
 
 static bool ParseAnalyzerArgs(AnalyzerOptions &Opts, ArgList &Args,
                               DiagnosticsEngine &Diags) {
   unsigned NumErrorsBefore = Diags.getNumErrors();