aaron.ballman added subscribers: sbc100, ruiu, lhames. aaron.ballman added inline comments.
================ Comment at: clang/docs/ReleaseNotes.rst:451 +- Add ``-Wcompare-function-pointers`` to warn about comparisons that may have + their behavior change when using ``icf=all``. ---------------- This makes it more clear this is an lld-specific change. ================ 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; ---------------- 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. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7007 + "comparison of function pointers (%0 and %1)">, + InGroup<CompareFunctionPointers>, DefaultIgnore; def warn_typecheck_ordered_comparison_of_function_pointers : Warning< ---------------- cjdb wrote: > cjdb wrote: > > It's very rare that we set a warning to `DefaultIgnore`. What's the > > motivation for this? > > This warning is disabled by default, since it's only relevant if ICF is > > explicitly enabled. > > I see why now. Perhaps this warning should be enabled by default when ICF is > also enabled, and an error otherwise. The problem is: ICF is an lld thing, it's not a Clang thing; so Clang has no idea if ICF will or won't be enabled. 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
