Sunil_Srivastava added inline comments.
================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6729 def err_no_dynamic_cast_with_fno_rtti : Error< - "cannot use dynamic_cast with -fno-rtti">; + "use of dynamic_cast requires enabling RTTI">; ---------------- wristow wrote: > probinson wrote: > > wristow wrote: > > > Sunil_Srivastava wrote: > > > > probinson wrote: > > > > > filcab wrote: > > > > > > I'd prefer to have the way to enable RTTI mentioned in the message. > > > > > > Could we have something like `ToolChain::getRTTIMode()` and have a > > > > > > "RTTI was on/of" or "RTTI defaulted to on/off"? That way we'd be > > > > > > able to have a message similar to the current one (mentioning "you > > > > > > passed -fno-rtti") on platforms that default to RTTI=on, and have > > > > > > your updated message (possibly with a mention of "use -frtti") on > > > > > > platforms that default to RTTI=off. > > > > > > > > > > > > (This is a minor usability comment about this patch, I don't > > > > > > consider it a blocker or anything) > > > > > If the options are spelled differently for clang-cl and we had a way > > > > > to retrieve the appropriate spellings, providing the option to use in > > > > > the diagnostic does seem like a nice touch. > > > > The idea of suggestion as to how-to-turn-on-rtti is appealing, but it > > > > is not trivial. > > > > > > > > First, clang-cl does not give this warning at all, so the issue is moot > > > > for clang-cl. > > > > > > > > For unix-line command-line, if the default RTTI mode in ENABLED (the > > > > unknown-linux) > > > > then this warning appears only if the user gives -fno-rtti options, so > > > > again we do > > > > not need to say anything more. > > > > > > > > The only cases left are compilers a where the default RTTI mode is > > > > DISABLED. > > > > Here an addendum like '[-frtti]' may make sense. AFAIK, PS4 is the only > > > > compiler > > > > with this default, but there may be other such private compilers. > > > > > > > > So should we append '[-frtti]' if Target.getTriple().isPS4() is true? > > > > So should we append '[-frtti]' if Target.getTriple().isPS4() is true? > > > > > > Personally, I'd be OK with producing a suggestion of how to enable RTTI > > > based on the PS4 triple. But I'd also be OK with not enhancing this > > > diagnostic to suggest how to turn on RTTI (that is, going with the patch > > > as originally proposed here). > > > > > > If clang-cl produced a warning when a dynamic_cast or typeid construct > > > was encountered in `/GR-` mode, then I'd feel it's worth complicating the > > > code to provide a target-sensitive way for advising the user how to turn > > > RTTI on. But clang-cl doesn't produce a warning in that case, so the > > > effort to add the framework for producing a target-sensitive warning > > > doesn't seem worth it to me. > > > > > > Improving clang-cl to produce a diagnostic in this `/GR-` situation seems > > > like a good idea, but separate from this proposed patch. If that work > > > gets done at some point, then it would be natural to revisit this > > > diagnostic at that time. > > If clang-cl is not a consideration, then I think the easiest and clearest > > way to do this is simply to say `requires -frtti` without hair-splitting > > which targets default which way. > Saying `requires -frtti` makes good sense to me. Done https://reviews.llvm.org/D47291 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
