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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

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



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:881
+  // Sort options by key to avoid relying on StringMap iteration order.
+  SmallVector<std::tuple<llvm::StringRef, std::string>, 4> SortedConfigOpts;
   for (const auto &C : Opts.Config) {
----------------
I think you should be able to drop `llvm::`.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:881
+  // Sort options by key to avoid relying on StringMap iteration order.
+  SmallVector<std::tuple<llvm::StringRef, std::string>, 4> SortedConfigOpts;
   for (const auto &C : Opts.Config) {
----------------
jansvoboda11 wrote:
> I think you should be able to drop `llvm::`.
No reason to allocate for this temporary storage IMO, you could replace `std::string` with `StringRef`.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:882-884
   for (const auto &C : Opts.Config) {
+    SortedConfigOpts.emplace_back(C.getKey(), C.getValue());
+  }
----------------
Per our coding style, we don't put braces around bodies with just a single statement.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:889
+
+  for (const auto &C : SortedConfigOpts) {
+    const llvm::StringRef &Key = std::get<0>(C);
----------------
You could use C++17 structured bindings: `for (const auto &[Key, Value] : SortedConfigOpts) { ... }`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142861/new/

https://reviews.llvm.org/D142861