In preparation for configured macro replacements in formatting,
add the ability to insert tokens to FormatTokenSource, and implement
token insertion in IndexedTokenSource.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Only serious concern is getPreviousToken().
clang/lib/Format/FormatTokenSource.h | ||
---|---|---|
81 | As I understand it, this parameter is used:
This simplifies the implementation, my only problem with this is it seems unusual and confusing. Alternatively you could consider slightly different data structures just for the purpose of making interfaces more obvious: e.g. pass in a BumpPtrAllocator&, allocate scratch tokens there, and use pointers instead of indices (jumps become a ptr->ptr map etc) | |
102 | this no longer seems valid, immediately after calling insertTokens(), Position is at the start of a jump range, and Tokens[Position - 1] will be the EOF at the end of the main stream or previous jump range. if this is never going to happen, you can detect it (I don't think Tokens[Position - 1] can be EOF in any other way) | |
123 | maybe add a comment that positions don't have meaningful order and this is only useful to restore the position? (All the callsites look good, it just feels like a trap) | |
138 | nit: move into the LLVM_DEBUG block below? or are you worried about a crash? | |
159 | nit: const I wasn't sure if this method advanced or not. "successor" might be clearer in this respect | |
181 | DenseMap? |
clang/lib/Format/FormatTokenSource.h | ||
---|---|---|
81 | Oops, somehow I missed that this was an array of *pointers* and assumed that copying it was no good. This is much better. | |
clang/lib/Format/UnwrappedLineParser.h | ||
283 | I'm not really sure how to read the combination of these two FIXMEs... does it mean "we wanted to do this differently one day, but now we never can"? Maybe either delete both, or if you think it's still potentially actionable, something like FIXME: it would be better to have tokens created and owned outside because XYZ, but it's hard because macro expansion mutates the stream (I don't really understand what the prev comment is about: the tokens *are* handed into the UnwrappedLineParser constructor. So I may be off base here) | |
clang/unittests/Format/FormatTokenSourceTest.cpp | ||
42 | nit: parens on (Tok) are redundant given Tok is a local rather than the macro arg |
Address review comments.
clang/lib/Format/UnwrappedLineParser.h | ||
---|---|---|
283 | Yeah, I think I don't fully understand what I wanted to fix, so deleted both. |
As I understand it, this parameter is used:
This simplifies the implementation, my only problem with this is it seems unusual and confusing.
A comment explaining the roles of this Tokens param would help a bit.
Alternatively you could consider slightly different data structures just for the purpose of making interfaces more obvious: e.g. pass in a BumpPtrAllocator&, allocate scratch tokens there, and use pointers instead of indices (jumps become a ptr->ptr map etc)