mstorsjo added a comment. 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. 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
