[PATCH] D123889: [clang-tidy] Improve macro handling in modernize-macro-to-enum

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Generally looks correct to me, but I did have some questions about the types used in the fix.



================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:235
   SmallVector<FileState> Files;
+  std::vector<std::string> ExpressionNames;
   FileState *CurrentFile = nullptr;
----------------
This smells expensive (compared to a `SmallVector<StringRef>` or somesuch).


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:303-305
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+    ExpressionNames.insert(Pos, Id);
+  }
----------------
So you're manually keeping the vector sorted instead of using a set?

Any chance that we can store a `StringRef` rather than paying the expense of all these copies? And would it make sense to switch to using an ordered container rather than an unordered one?


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

https://reviews.llvm.org/D123889