adriandole 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; ---------------- aaron.ballman wrote: > 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. ld.gold and mold have it (same name, `-icf=all`) as does MSVC (`/OPT:ICF`). 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
