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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
