[PATCH] D18172: [CUDA][OpenMP] Add a generic offload action builder

Samuel Antao via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 30 21:16:41 PDT 2016

sfantao added a comment.

In http://reviews.llvm.org/D18172#471861, @jlebar wrote:

> Alexey, it seems that you're asking for "final" on all classes that are not inherited from.  Forgive my ignorance, but would you mind pointing me to the document that talks about our position on "final" in LLVM source?  I don't see it in the style guide, but I may be missing something.
> The style guide does talk a good bit about writing concise and generally not-misleading code.  My concern is that adding "final" everywhere paints an inaccurate picture and will mislead readers.  Specifically, "final" is useful as a signal to readers that a class cannot safely be inherited from.  "Don't even think about it, buster."  But here we're adding "final" to a lot of classes that, as far as I can tell, aren't distinctive except in that they have no subclasses today.  The problem with this is that, if we use "final" in this way, it dilutes the first "don't even try" meaning.  Now when I see a class with "final" that I want to subclass, I'm just going to rip the "final" off, because chances are, I can do so safely.  Now "final" does not serve as a warning to me that I shouldn't do this.
> Sorry to focus on a superficial issue, but I think this really does matter for usability.

Hi Justin,

Thanks for the comment!

I understand both interpretations can be useful for different purposes, i.e. "don't inherit, it is not safe" vs "feel free to change this class without worrying with subclasses". I tend to think that the latter (which I believe is what Alexey has in mind) can, in general, be more useful in the sense that a class is usually safe to extend - you can always add features that are somewhat orthogonal to what the parent class is doing. I can, of course, imagine cases where your argument is valid - you can have an implementation that allocates memory based on the type of the parent therefore it does not observe the extra storage required by derived classes. However, I think these cases are less common.

I also look at the programming guidelines and couldn't find a clear answer to the issue you raised.

I don't have a strong opinion about this, just giving my two cents.

Thanks again!


More information about the cfe-commits mailing list