Re: [PATCH] D101192: Add support for #elifdef and #elifndef

On Thu, Oct 21, 2021 at 2:40 PM Richard Smith - zygoloid via Phabricator wrote: > > rsmith added a comment. > > > In this version of the patch, I am supporting the feature in all C and C++ > > modes. This seems like more useful functionality than diagnosing an unknown > > preprocessor directive

[PATCH] D101192: Add support for #elifdef and #elifndef

rsmith added a comment. > In this version of the patch, I am supporting the feature in all C and C++ > modes. This seems like more useful functionality than diagnosing an unknown > preprocessor directive in older language modes, and is a conforming extension > in those modes anyway as use of an

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:594 +if (CondInfo.FoundElse) + Diag(Tok, diag::pp_err_elif_after_else) << PED_Elif; bjope wrote: > aaron.ballman wrote: > > bjope wrote: > > > Hi @aaron.ballman >

[PATCH] D101192: Add support for #elifdef and #elifndef

bjope added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:594 +if (CondInfo.FoundElse) + Diag(Tok, diag::pp_err_elif_after_else) << PED_Elif; aaron.ballman wrote: > bjope wrote: > > Hi @aaron.ballman > > > > This change is missi

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:594 +if (CondInfo.FoundElse) + Diag(Tok, diag::pp_err_elif_after_else) << PED_Elif; bjope wrote: > Hi @aaron.ballman > > This change is missing from > https://re

[PATCH] D101192: Add support for #elifdef and #elifndef

bjope added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:594 +if (CondInfo.FoundElse) + Diag(Tok, diag::pp_err_elif_after_else) << PED_Elif; Hi @aaron.ballman This change is missing from https://reviews.llvm.org/rG8edd3464afbf

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman closed this revision. aaron.ballman added a comment. I've landed in 8edd3464afbff65d7d5945b3a8b20009d6ff5deb CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101192/new/ https://reviews.llvm.org/D101192 _

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman added a comment. In D101192#2763498 , @mibintc wrote: > let 'er rip Thanks @mibintc! @rsmith -- I'll land this sometime later this week unless I hear further comments from you. If I land it and you still have feedback, I can address it p

[PATCH] D101192: Add support for #elifdef and #elifndef

mibintc accepted this revision. mibintc added a comment. This revision is now accepted and ready to land. let 'er rip CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101192/new/ https://reviews.llvm.org/D101192 ___ cfe-commits mailing list cfe-

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101192/new/ https://reviews.llvm.org/D101192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman updated this revision to Diff 343731. aaron.ballman marked an inline comment as done. aaron.ballman added a comment. Now, with ever-so-slightly better formatting (the rest of the formatting issues are either matching existing style or a fight between my clang-format and CI's clang-

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman marked 2 inline comments as done. aaron.ballman added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:3254-3265 + Token MacroNameTok; + ReadMacroName(MacroNameTok); + + // Error reading macro name? If so, diagnostic already issued. + if (MacroNameTo

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman updated this revision to Diff 343680. aaron.ballman added a comment. Updated based on review feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101192/new/ https://reviews.llvm.org/D101192 Files: clang/include/clang/Basic/DiagnosticLexKinds.td clang/include/clang

[PATCH] D101192: Add support for #elifdef and #elifndef

rsmith added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:3254-3265 + Token MacroNameTok; + ReadMacroName(MacroNameTok); + + // Error reading macro name? If so, diagnostic already issued. + if (MacroNameTok.is(tok::eod)) { +// Skip code until we get to #en

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101192/new/ https://reviews.llvm.org/D101192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman added a comment. In D101192#2716596 , @mibintc wrote: > Wow thanks for doing this! I worked on it a couple days a while ago but I > abandoned the effort and went back to my day job. Happy to help! > It seems like preprocessing ought to be

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman updated this revision to Diff 340797. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Updating based on review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101192/new/ https://reviews.llvm.org/D101192 Files: clang/include/clang/

[PATCH] D101192: Add support for #elifdef and #elifndef

mibintc added a comment. Wow thanks for doing this! I worked on it a couple days a while ago but I abandoned the effort and went back to my day job. It seems like preprocessing ought to be something like a "state machine" but I couldn't figure out the mechanism. Would it make sense to add some

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman added inline comments. Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:909 If, // if/ifdef/ifndef Else // elif,else }; dexonsmith wrote: > Please add the two new cases to this comment as well. Good catch, thanks

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman updated this revision to Diff 340482. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Updates based on review feedback. Added tests for the source minimizer changes, thanks @dexonsmith! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101192/new

[PATCH] D101192: Add support for #elifdef and #elifndef

dexonsmith added a comment. I reviewed the source minimizer sections, and the code looks correct. It'd probably be good to have unit tests in clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp for the incremental changes. - Do the new constructs stick around in the minimized sourc

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:621 +if (CondInfo.FoundElse) +Diag(Tok, diag::pp_err_elif_after_else) << (IsElifDef ? 1 : 2); + erichkeane wrote: > As you know, I'm a giant fan of using enums for

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman updated this revision to Diff 340283. aaron.ballman marked 2 inline comments as done. aaron.ballman added a comment. Updated based on review feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101192/new/ https://reviews.llvm.org/D101192 Files: clang/include/clang/B

[PATCH] D101192: Add support for #elifdef and #elifndef

erichkeane added a comment. I have one 'nit of preference', otherwise I think I don't want to +1 this without giving people a few days to take a look. Based on my looks through the surrounding code, this _LOOKS_ right enough as far as I can tell, but I still want to give others a few days.

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman updated this revision to Diff 340157. aaron.ballman marked 4 inline comments as done. aaron.ballman added a comment. Updated based on review feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101192/new/ https://reviews.llvm.org/D101192 Files: clang/include/clang/B

[PATCH] D101192: Add support for #elifdef and #elifndef

erichkeane added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:642 + + CheckEndOfDirective(IsElifDef ? "elifdef" : "elifndef"); + aaron.ballman wrote: > erichkeane wrote: > > Can you just pass 'Directive' here? > I think I'd have to pass `D

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman marked 4 inline comments as done. aaron.ballman added inline comments. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:754 break; + case tok::pp_elifdef: + case tok::pp_elifndef: erichkeane wrote: > Just looking through this, so forgive

[PATCH] D101192: Add support for #elifdef and #elifndef

erichkeane added inline comments. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:754 break; + case tok::pp_elifdef: + case tok::pp_elifndef: Just looking through this, so forgive me if I there is something I don't understand... Why is this not doing

[PATCH] D101192: Add support for #elifdef and #elifndef

aaron.ballman created this revision. aaron.ballman added reviewers: rsmith, rjmccall, mibintc, erichkeane. Herald added subscribers: dexonsmith, arphaman, kbarton, nemanjai. aaron.ballman requested review of this revision. Herald added a project: clang. WG14 adopted N2645 and WG21 EWG has accepted