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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
