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.
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
