[PATCH] D47291: Proposal to make rtti errors more generic.

[Paul Robinson via Phabricator via cfe-commits]

probinson 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:
> 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.


Repository:
  rC Clang

https://reviews.llvm.org/D47291