On Mar 4, 2014, at 7:57 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:> On 2014 Mar 4, at 15:01, Sean Silva <chisophugis at gmail.com> wrote: > >> On Mon, Mar 3, 2014 at 2:02 AM, David Blaikie <dblaikie at gmail.com> wrote: >> On Sun, Mar 2, 2014 at 8:19 PM, Craig Topper <craig.topper at gmail.com> wrote: >>> While doing the conversion of LLVM_OVERRIDE to 'override' last night, I >>> noticed that the code base is rather inconsistent on whether the 'virtual' >>> keyword is also used when 'override' is used. >>> >>> Should we have a coding standard for this? What's the preferred direction >>> here? Seems not having 'virtual' is less overall text, but not sure how >>> others feel. >> >> My vote: omit virtual if override is used. > > +1: virtual doesn’t add anything if override is present. > >> (legitimate counterargument: harder to skim/match/read whether a >> function is virtual when it's not specified and "override" appears >> much later in the declaration) >> >> One counter-datapoint: Personally, I have on at least one occasion caught myself not noticing a leading `virtual` and thinking that a method wasn't overriden because of the missing `override`. I guess the moral is that this can be pretty adaptable. >> >> FWIW IMO the preferred end state is to have no useless leading `virtual`'s and using `override` for its intended purpose. >> >> -- Sean Silva >> >> >>> Related, should we require use of 'override' when methods override a base >>> class method? >> >> My vote: require override. > > +1: override is useful and prevents errors.Would it be too much to have clang emit a warning/error if override is missing? I know that sounds crazy and people hate errors which fire too often, but there’s not too much C++11 code out there yet, and so we have a chance to put errors/warnings in now without too much pain. People might just get used to them and think its how code has to be written :)> >>> I have clang-tidy checks for both though haven't implemented fixits for them >>> yet. >> >> Cool. There has also been a suggestion that Clang could warn about >> omitted override if at least one other member function in the same >> class is marked override, which could get us a lot of value built into >> the compiler (but not 'all the way', so a clang-tidy check would still >> be appropriate). >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140304/6192e50d/attachment.html>
On Tue, Mar 4, 2014 at 10:38 PM, Pete Cooper <peter_cooper at apple.com> wrote:> Would it be too much to have clang emit a warning/error if override is > missing? I know that sounds crazy and people hate errors which fire too > often, but there’s not too much C++11 code out there yet, and so we have a > chance to put errors/warnings in now without too much pain. People might > just get used to them and think its how code has to be written :) >There is already work on a clang-tidy check for this. We actually have the framework for Chris's asked-for coding convention checker. Now we get to use it. =D -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140304/19c3f6b9/attachment.html>
> On Mar 4, 2014, at 22:38, Pete Cooper <peter_cooper at apple.com> wrote: > > >> On Mar 4, 2014, at 7:57 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote: >> >>> On 2014 Mar 4, at 15:01, Sean Silva <chisophugis at gmail.com> wrote: >>> >>> On Mon, Mar 3, 2014 at 2:02 AM, David Blaikie <dblaikie at gmail.com> wrote: >>>> On Sun, Mar 2, 2014 at 8:19 PM, Craig Topper <craig.topper at gmail.com> wrote: >>>> While doing the conversion of LLVM_OVERRIDE to 'override' last night, I >>>> noticed that the code base is rather inconsistent on whether the 'virtual' >>>> keyword is also used when 'override' is used. >>>> >>>> Should we have a coding standard for this? What's the preferred direction >>>> here? Seems not having 'virtual' is less overall text, but not sure how >>>> others feel. >>> >>> My vote: omit virtual if override is used. >> >> +1: virtual doesn’t add anything if override is present. >> >>> (legitimate counterargument: harder to skim/match/read whether a >>> function is virtual when it's not specified and "override" appears >>> much later in the declaration) >>> >>> One counter-datapoint: Personally, I have on at least one occasion caught myself not noticing a leading `virtual` and thinking that a method wasn't overriden because of the missing `override`. I guess the moral is that this can be pretty adaptable. >>> >>> FWIW IMO the preferred end state is to have no useless leading `virtual`'s and using `override` for its intended purpose. >>> >>> -- Sean Silva >>> >>> >>>> Related, should we require use of 'override' when methods override a base >>>> class method? >>> >>> My vote: require override. >> >> +1: override is useful and prevents errors. > Would it be too much to have clang emit a warning/error if override is missing? I know that sounds crazy and people hate errors which fire too often, but there’s not too much C++11 code out there yet, and so we have a chance to put errors/warnings in now without too much pain. People might just get used to them and think its how code has to be written :)Might be a nightmare when including legacy headers, but warnings can always be disabled...>> >>>> I have clang-tidy checks for both though haven't implemented fixits for them >>>> yet. >>> >>> Cool. There has also been a suggestion that Clang could warn about >>> omitted override if at least one other member function in the same >>> class is marked override, which could get us a lot of value built into >>> the compiler (but not 'all the way', so a clang-tidy check would still >>> be appropriate). >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140304/b89ec8b5/attachment.html>
On Mar 4, 2014, at 10:48 PM, Duncan Exon Smith <dexonsmith at apple.com> wrote:>>>> >>>>> Related, should we require use of 'override' when methods override a base >>>>> class method? >>>> >>>> My vote: require override. >>> >>> +1: override is useful and prevents errors. >> Would it be too much to have clang emit a warning/error if override is missing? I know that sounds crazy and people hate errors which fire too often, but there’s not too much C++11 code out there yet, and so we have a chance to put errors/warnings in now without too much pain. People might just get used to them and think its how code has to be written :) > > Might be a nightmare when including legacy headers, but warnings can always be disabled...A clang warning for this would be awesome, but it should be off by default. That said, the build of LLVM itself could detect that Clang had this warning and turn it on. I think it would be great to have the makefiles/cmake detect modern clang's and turn on additional warnings that we can't inflict on the world by default. -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140304/b45941fa/attachment.html>
On Tue, Mar 04, 2014 at 10:48:49PM -0800, Duncan Exon Smith wrote:> Might be a nightmare when including legacy headers, but warnings can always be disabled...What about tagging the class as requiring override? Could be an attribute or pragma... Joerg