LegalizeAdulthood marked 3 inline comments as done. LegalizeAdulthood added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 + CRLF, + CRLFCR, + }; ---------------- aaron.ballman wrote: > LegalizeAdulthood wrote: > > aaron.ballman wrote: > > > LegalizeAdulthood wrote: > > > > aaron.ballman wrote: > > > > > LegalizeAdulthood wrote: > > > > > > aaron.ballman wrote: > > > > > > > I'm a bit confused by this one as this is not a valid line ending > > > > > > > (it's either three valid line endings or two valid line endings, > > > > > > > depending on how you look at it). Can you explain why this is > > > > > > > needed? > > > > > > It's a state machine, where the states are named for what we've > > > > > > seen so far and we're looking for //two// consecutive line endings, > > > > > > not just one. Does it make sense now? > > > > > Thanks, I understood it was a state machine, but it's a confused one > > > > > to me. `\r` was the line ending on Mac Classic, I've not seen it used > > > > > outside of that platform (and I've not seen anyone write code for > > > > > that platform in a long time). So, to me, the only valid combinations > > > > > of line endings to worry about are: `LF LF`; `CRLF CRLF`; `CRLF LF`; > > > > > `LF CRLF`. > > > > > > > > > > `LF LF` returns false (Nothing -> LF -> return false) > > > > > `CRLF CRLF` returns false (Nothing -> CR -> CRLF -> CRLFCR -> return > > > > > false) > > > > > `CRLF LF` returns true (Nothing -> CR -> CRLF -> LF -> finish loop) > > > > > `LF CRLF` returns true (Nothing -> LF -> CR -> CRLF -> finish loop) > > > > > > > > > > (If you also intend to support Mac Classic line endings for some > > > > > reason, this gets even more complicated.) > > > > I was trying to follow "be liberal in what you accept as input and > > > > conservative in what you generate as output" maxim. I can remove the > > > > `CR` as a line ending case if you think it's too obscure. > > > If Clang supports it as a line ending, we probably should too, but... how > > > do we handle CRLF vs "I mixed a CR with an LF by accident" kind of > > > inputs? (Maybe we just treat that as CRLF and if the behavior is bad, the > > > user shouldn't mix their line endings that way; I think that's > > > defensible.) That seems to be similar to the scenario that's confusing me > > > above where the user mixed an LF and CRLF by accident. > > Well, as far as Clang is concerned it's all just "whitespace" that gets > > eaten up by the preprocessor. Actually, that gives me a thought. A > > preprocessing directive is considered to end at the physical line ending, > > so I should look to see what sort of characters it considers to "end the > > line". > > > > For the accidental mix-up, I'm not going to worry about that here. Your > > input files are assumed to be "well formed". The worst that happens in > > this check is that two blocks of macros that //look// like they are > > separated by a blank line are considered as a single clump by this check. > > > > In other words, the worst that can happen is: > > - Two clumps of macros are considered together. > > - One clump of macros that is discarded because it doesn't follow the > > constraints "taints" an adjacent clump of macros that do follow the > > constraints. > > > > Either way, nothing harmful happens to your code. It will still compile > > and be syntactically and semantically equivalent to what was there before. > > > > Actually, that gives me a thought. A preprocessing directive is considered > > to end at the physical line ending, so I should look to see what sort of > > characters it considers to "end the line". > > All of `\r`, `\n`, `\r\n` I believe (you can double-check in > `Lexer::LexTokenInternal()` > > > Either way, nothing harmful happens to your code. It will still compile and > > be syntactically and semantically equivalent to what was there before. > > Oh, that's a very good point, thank you. I think that's reasonable fallback > behavior for these weird edge cases. Well..... maybe. If you look at `Lexer::ReadToEndOfLine` which is used to skip to the end of a preprocessor directive you'll see that it considers the first of `'\r'`, `'\n'` or `'\0'` (end of file) as the end of the "line". This is around line 2835 of Lexer.cpp in my tree. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
