aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
In D129277#3637317 <https://reviews.llvm.org/D129277#3637317>, @mstorsjo wrote: > In D129277#3636795 <https://reviews.llvm.org/D129277#3636795>, @aaron.ballman > wrote: > >> In D129277#3636596 <https://reviews.llvm.org/D129277#3636596>, @mstorsjo >> wrote: >> >>> In D129277#3636567 <https://reviews.llvm.org/D129277#3636567>, >>> @aaron.ballman wrote: >>> >>>> Thanks for catching this! Is it really an NFC change though (it seems like >>>> it would change some of the diagnostic behavior and the list of suggested >>>> predefines)? Can you add test coverage for the change? >>> >>> TBH I haven’t tried to follow exactly where this case would matter in the >>> current state of affairs - the function is called in three places, and >>> maybe the individual roles of the parameters currently only make a >>> difference in the other callers. As it didn’t break any tests I presumed >>> it’s NFC. >> >> Heh, I presumed we just lacked test coverage. :-) But I also don't know >> enough about this interface to know exactly how to test it. I would imagine >> that this would be caught through using a PCH that was compiled with >> different preprocessor options than the code consuming the header. I'm not >> certain if `-D` or `-U` is sufficient to demonstrate the issue or not, but >> maybe `-fallow-editor-placeholders` and `-fno-allow-editor-placeholders` >> would work? > > The thing is that this particular caller passes `nullptr` for `Diags`, so no > diagnostics are emitted, and the generated `SuggestedPredefines` string is > ignored by the one caller (`ASTReader::isAcceptableASTFile`). So within that > function, the only thing that is returned is a boolean for whether the two > (AST file and command line parameters) are compatible, and in that respect, > the two parameters are commutative - so I think it's not possible to observe > whether these two parameters are passed correctly or not, right now. Hence > NFC. Ah, I see it now, this really is an NFC change, thank you for the explanation. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129277/new/ https://reviews.llvm.org/D129277 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
