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