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

Richard via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 19 09:14:19 PDT 2022

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added inline comments.

Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:235
   SmallVector<FileState> Files;
+  std::vector<std::string> ExpressionNames;
   FileState *CurrentFile = nullptr;
aaron.ballman wrote:
> This smells expensive (compared to a `SmallVector<StringRef>` or somesuch).
Initially I had StringRef, but the problem is that the lifetime of those string references doesn't last long enough.  From StringRef docs:

> This class does not own the string data, it is expected to be used in situations where the character data resides in some other buffer, whose lifetime extends past that of the StringRef. For this reason, it is not in general safe to store a StringRef.

Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:303-305
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+    ExpressionNames.insert(Pos, Id);
+  }
aaron.ballman wrote:
> 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?
Can't store a `StringRef` because the underlying data doesn't live long enough (I tried it).

Yes, I was keeping a sorted array.  I can switch it to a set; I guess memory locality doesn't really matter much in this case because we've already got `std::string` throwing things on the heap.

Let me know what you think is best, I'm ambivalent about it.



More information about the cfe-commits mailing list