[PATCH] D141310: [clang] add -Wcompare-function-pointers

Wed Jan 11 10:20:49 PST 2023

aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7006
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup<CompareFunctionPointers>, DefaultIgnore;
----------------
adriandole wrote:
> aaron.ballman wrote:
> > cjdb wrote:
> > > This diagnostic feels very bare bones, and doesn't explain why the user should care. It would be good to rephrase the message so that it can incorporate some kind of reason too.
> > Agreed -- diagnostic wording is usually trying to tell the user what they did wrong to guide them how to fix it, but this reads more like the diagnostic explains what the code does. This one will be especially interesting because people would naturally expect that comparison to work per the standard. And if enabling ICF breaks that then ICF is non-conforming because it makes valid code silently invalid. I think this is an ICF bug (it may still be worth warning users about though because you can use newer Clang with an older lld and hit the issue even if lld addresses this issue).
> > 
> > CC @lhames @sbc100 @ruiu in to try to scare up an lld code owner to see if this is known and if there's something that can be done on the lld side to not break valid code.
> There's already an ICF option that doesn't break valid code: `-icf=safe`. Only if you explicitly decide that you don't care about the results of function pointer comparisons would you use `-icf=all`, which enables more code folding to be done than the safe option.
Ohhh interesting, so it's probably known that `-icf=all` will break code because it's not conforming and thus isn't an lld bug at all. Do (most?) other linkers also have the same functionality as `-icf=all`? I'm trying to understand whether this should be added to clang at all as it seems like it's a warning for a very particular usage pattern (a specific linker with a specific option ), which make it more reasonable for clang-tidy instead of the compiler.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141310/new/

https://reviews.llvm.org/D141310