aaron.ballman 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( ---------------- balazske wrote: > 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. > The function declaration is not often a macro name so there is no "expansion > location" or the same as the original location. Agreed. > 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. Typically, when a C user does something like that, it's because they're explicitly *not* including the header file at all (they're just forward declaring the function so they can use it) and so we'd get the logic wrong for them anyway because we'd never find the declaration in the system header. Using the canonical declaration is more typical and would realistically fail only in circumstances like forward declaring a system header function and then later including the system header itself. That said, I suppose your approach is defensible enough. Redeclaration chains are hopefully short enough that it isn't a performance hit. 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
