balazske added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:33 + + for (const FunctionDecl *D : Node.redecls()) + if (D->getASTContext().getSourceManager().isInSystemHeader( ---------------- aaron.ballman wrote: > balazske wrote: > > aaron.ballman wrote: > > > I'm not certain I understand why we're looking through the entire > > > redeclaration chain to see if the function is ever mentioned in a system > > > header. I was expecting we'd look at the expansion location of the > > > declaration and see if that's in a system header, which is already > > > handled by the `isExpansionInSystemHeader()` matcher. Similar below. > > This function is called from ` SignalHandlerCheck::check` when any function > > call is found. So the check for system header is needed. It was unclear to > > me what the "expansion location" means but it seems to work if using that > > expansion location and checking for system header, instead of this loop. I > > will update the code. > > This function is called from SignalHandlerCheck::check when any function > > call is found. So the check for system header is needed. > > The check for the system header isn't what I was concerned by, it was the > fact that we're walking the entire redeclaration chain to see if *any* > declaration is in a system header that I don't understand the purpose of. > > > It was unclear to me what the "expansion location" means but it seems to > > work if using that expansion location and checking for system header, > > instead of this loop. I will update the code. > > The spelling location is where the user wrote the code and the expansion > location is where the macro name is written, but thinking on it harder, that > shouldn't matter for this situation as either location would be in the system > header anyway. The function declaration is not often a macro name so there is no "expansion location" or the same as the original location. My concern was that if there is a declaration of system call function in a source file (like `void abort(void);` in .c file) for any reason, we may find this declaration instead of the one in the header file, if not looping over the declaration chain. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87449/new/ https://reviews.llvm.org/D87449 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
