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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
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:
> 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`.
Perhaps std::pair would be slightly easier to read than std::tuple?


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142861