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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 19 09:30:05 PDT 2022


aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM! Thank you for the fix!



================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:235
   SmallVector<FileState> Files;
+  std::vector<std::string> ExpressionNames;
   FileState *CurrentFile = nullptr;
----------------
LegalizeAdulthood wrote:
> 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.
> 
> 
> 
> 
Ah, I was thinking that the original string data was sufficiently long-lived for this to actually work, but you're right, that's just playing with fire. Sorry for the noise!


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:303-305
+  if (Pos == ExpressionNames.end() || *Pos != Id) {
+    ExpressionNames.insert(Pos, Id);
+  }
----------------
LegalizeAdulthood wrote:
> 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.
Yeah, I'm ambivalent about it now as well -- I'd say this is fine as-is.


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

https://reviews.llvm.org/D123889



More information about the cfe-commits mailing list