On Thu, Mar 6, 2014 at 3:47 PM, Craig Topper <craig.topper at gmail.com> wrote:> virtual bar *foo() = 0; > > where foo() also exists as pure in the base class. Clang-modernize has a > FIXME that says it can't find the "=0" to do the insert of override. >Does that mean we have a pure virtual function with implementation in Clang/LLVM? If so, I feel it's a little bit confusing. If not, we should remove such redundant pure virtual function declarations from derived classes, no? On Thu, Mar 6, 2014 at 3:00 PM, Rui Ueyama <ruiu at google.com> wrote:> >> On Thu, Mar 6, 2014 at 2:21 PM, Craig Topper <craig.topper at gmail.com>wrote: >> >>> It also doesn't do pure methods either. >> >> >> I think I don't quite understand what that means. Can you give me an >> example? >> >> >>> On Thursday, March 6, 2014, Rui Ueyama <ruiu at google.com> wrote: >>> >>>> After running the tool aginst LLD, I realized that clang-modernize do >>>> not add "override" to virtual destructors. I think it's not intended but >>>> just that that case is not covered by the tool. >>>> >>>> >>>> On Wed, Mar 5, 2014 at 2:54 PM, Craig Topper <craig.topper at gmail.com>wrote: >>>> >>>> Didn't realize that. I'll see if i can figure out how to make it delete >>>> the virtual keyword. >>>> >>>> >>>> On Wed, Mar 5, 2014 at 2:32 PM, Ben Langmuir <blangmuir at apple.com>wrote: >>>> >>>> clang-modernize has a -format option that will run clang-format on the >>>> code it changes. >>>> >>>> Ben >>>> >>>> >>>> >>>> On Mar 5, 2014, at 2:26 PM, Craig Topper <craig.topper at gmail.com> >>>> wrote: >>>> >>>> clang-modernize can add the 'override', but it can't currently delete >>>> 'virtual'. It will also potentially overflow 80 columns. And if it removed >>>> virtual it would fail to align a second line of arguments correctly. So you >>>> need modernize and clang-format I guess. Though I'm not sure we want to >>>> widespread apply clang-format. >>>> >>>> >>>> On Wed, Mar 5, 2014 at 1:10 PM, Rui Ueyama <ruiu at google.com> wrote: >>>> >>>> On Wed, Mar 5, 2014 at 1:02 PM, Marshall Clow <mclow.lists at gmail.com>wrote: >>>> >>>> >>>> On Mar 5, 2014, at 10:29 AM, Chris Lattner <clattner at apple.com> wrote: >>>> >>>> >>>> On Mar 5, 2014, at 9:53 AM, David Blaikie <dblaikie at gmail.com> wrote: >>>> >>>> It might be reasonable to warn if a class has both a function marked >>>> 'override' and a function that overrides but is not marked 'override'. >>>> >>>> >>>> That could be useful - because it means that the author of the class is >>>> at >>>> least thinking about override - but having a "coding style" warning of >>>> "I >>>> always intend to use override" would still be useful. >>>> >>>> >>>> Doug (not sure about other Clang owners) is pretty hesitant about >>>> implementing coding style warnings - anything with such a high false >>>> >>>> >>> >>> -- >>> ~Craig >>> >> >> > > > -- > ~Craig >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140306/8b49e987/attachment.html>
The cases I saw were changing the return types. For example: TerminatorInst *clone_impl() const override = 0; this overrides virtual Instruction *clone_impl() const = 0; On Thu, Mar 6, 2014 at 4:23 PM, Rui Ueyama <ruiu at google.com> wrote:> On Thu, Mar 6, 2014 at 3:47 PM, Craig Topper <craig.topper at gmail.com>wrote: > >> virtual bar *foo() = 0; >> >> where foo() also exists as pure in the base class. Clang-modernize has a >> FIXME that says it can't find the "=0" to do the insert of override. >> > > Does that mean we have a pure virtual function with implementation in > Clang/LLVM? If so, I feel it's a little bit confusing. If not, we should > remove such redundant pure virtual function declarations from derived > classes, no? > > On Thu, Mar 6, 2014 at 3:00 PM, Rui Ueyama <ruiu at google.com> wrote: >> >>> On Thu, Mar 6, 2014 at 2:21 PM, Craig Topper <craig.topper at gmail.com>wrote: >>> >>>> It also doesn't do pure methods either. >>> >>> >>> I think I don't quite understand what that means. Can you give me an >>> example? >>> >>> >>>> On Thursday, March 6, 2014, Rui Ueyama <ruiu at google.com> wrote: >>>> >>>>> After running the tool aginst LLD, I realized that clang-modernize do >>>>> not add "override" to virtual destructors. I think it's not intended but >>>>> just that that case is not covered by the tool. >>>>> >>>>> >>>>> On Wed, Mar 5, 2014 at 2:54 PM, Craig Topper <craig.topper at gmail.com>wrote: >>>>> >>>>> Didn't realize that. I'll see if i can figure out how to make it >>>>> delete the virtual keyword. >>>>> >>>>> >>>>> On Wed, Mar 5, 2014 at 2:32 PM, Ben Langmuir <blangmuir at apple.com>wrote: >>>>> >>>>> clang-modernize has a -format option that will run clang-format on the >>>>> code it changes. >>>>> >>>>> Ben >>>>> >>>>> >>>>> >>>>> On Mar 5, 2014, at 2:26 PM, Craig Topper <craig.topper at gmail.com> >>>>> wrote: >>>>> >>>>> clang-modernize can add the 'override', but it can't currently delete >>>>> 'virtual'. It will also potentially overflow 80 columns. And if it removed >>>>> virtual it would fail to align a second line of arguments correctly. So you >>>>> need modernize and clang-format I guess. Though I'm not sure we want to >>>>> widespread apply clang-format. >>>>> >>>>> >>>>> On Wed, Mar 5, 2014 at 1:10 PM, Rui Ueyama <ruiu at google.com> wrote: >>>>> >>>>> On Wed, Mar 5, 2014 at 1:02 PM, Marshall Clow <mclow.lists at gmail.com>wrote: >>>>> >>>>> >>>>> On Mar 5, 2014, at 10:29 AM, Chris Lattner <clattner at apple.com> wrote: >>>>> >>>>> >>>>> On Mar 5, 2014, at 9:53 AM, David Blaikie <dblaikie at gmail.com> wrote: >>>>> >>>>> It might be reasonable to warn if a class has both a function marked >>>>> 'override' and a function that overrides but is not marked 'override'. >>>>> >>>>> >>>>> That could be useful - because it means that the author of the class >>>>> is at >>>>> least thinking about override - but having a "coding style" warning of >>>>> "I >>>>> always intend to use override" would still be useful. >>>>> >>>>> >>>>> Doug (not sure about other Clang owners) is pretty hesitant about >>>>> implementing coding style warnings - anything with such a high false >>>>> >>>>> >>>> >>>> -- >>>> ~Craig >>>> >>> >>> >> >> >> -- >> ~Craig >> > >-- ~Craig -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140306/8a0bd6dd/attachment.html>
Looking a little closer StreamableMemoryObject.h does have some seemingly redundant overrides. And this override of a non-pure virtual with a pure virtual virtual int readBytes(uint64_t address, uint64_t size, uint8_t *buf) const override = 0; On Thu, Mar 6, 2014 at 7:27 PM, Craig Topper <craig.topper at gmail.com> wrote:> The cases I saw were changing the return types. For example: > > TerminatorInst *clone_impl() const override = 0; > > this overrides > > virtual Instruction *clone_impl() const = 0; > > > On Thu, Mar 6, 2014 at 4:23 PM, Rui Ueyama <ruiu at google.com> wrote: > >> On Thu, Mar 6, 2014 at 3:47 PM, Craig Topper <craig.topper at gmail.com>wrote: >> >>> virtual bar *foo() = 0; >>> >>> where foo() also exists as pure in the base class. Clang-modernize has a >>> FIXME that says it can't find the "=0" to do the insert of override. >>> >> >> Does that mean we have a pure virtual function with implementation in >> Clang/LLVM? If so, I feel it's a little bit confusing. If not, we should >> remove such redundant pure virtual function declarations from derived >> classes, no? >> >> On Thu, Mar 6, 2014 at 3:00 PM, Rui Ueyama <ruiu at google.com> wrote: >>> >>>> On Thu, Mar 6, 2014 at 2:21 PM, Craig Topper <craig.topper at gmail.com>wrote: >>>> >>>>> It also doesn't do pure methods either. >>>> >>>> >>>> I think I don't quite understand what that means. Can you give me an >>>> example? >>>> >>>> >>>>> On Thursday, March 6, 2014, Rui Ueyama <ruiu at google.com> wrote: >>>>> >>>>>> After running the tool aginst LLD, I realized that clang-modernize do >>>>>> not add "override" to virtual destructors. I think it's not intended but >>>>>> just that that case is not covered by the tool. >>>>>> >>>>>> >>>>>> On Wed, Mar 5, 2014 at 2:54 PM, Craig Topper <craig.topper at gmail.com>wrote: >>>>>> >>>>>> Didn't realize that. I'll see if i can figure out how to make it >>>>>> delete the virtual keyword. >>>>>> >>>>>> >>>>>> On Wed, Mar 5, 2014 at 2:32 PM, Ben Langmuir <blangmuir at apple.com>wrote: >>>>>> >>>>>> clang-modernize has a -format option that will run clang-format on >>>>>> the code it changes. >>>>>> >>>>>> Ben >>>>>> >>>>>> >>>>>> >>>>>> On Mar 5, 2014, at 2:26 PM, Craig Topper <craig.topper at gmail.com> >>>>>> wrote: >>>>>> >>>>>> clang-modernize can add the 'override', but it can't currently delete >>>>>> 'virtual'. It will also potentially overflow 80 columns. And if it removed >>>>>> virtual it would fail to align a second line of arguments correctly. So you >>>>>> need modernize and clang-format I guess. Though I'm not sure we want to >>>>>> widespread apply clang-format. >>>>>> >>>>>> >>>>>> On Wed, Mar 5, 2014 at 1:10 PM, Rui Ueyama <ruiu at google.com> wrote: >>>>>> >>>>>> On Wed, Mar 5, 2014 at 1:02 PM, Marshall Clow <mclow.lists at gmail.com>wrote: >>>>>> >>>>>> >>>>>> On Mar 5, 2014, at 10:29 AM, Chris Lattner <clattner at apple.com> >>>>>> wrote: >>>>>> >>>>>> >>>>>> On Mar 5, 2014, at 9:53 AM, David Blaikie <dblaikie at gmail.com> wrote: >>>>>> >>>>>> It might be reasonable to warn if a class has both a function marked >>>>>> 'override' and a function that overrides but is not marked 'override'. >>>>>> >>>>>> >>>>>> That could be useful - because it means that the author of the class >>>>>> is at >>>>>> least thinking about override - but having a "coding style" warning >>>>>> of "I >>>>>> always intend to use override" would still be useful. >>>>>> >>>>>> >>>>>> Doug (not sure about other Clang owners) is pretty hesitant about >>>>>> implementing coding style warnings - anything with such a high false >>>>>> >>>>>> >>>>> >>>>> -- >>>>> ~Craig >>>>> >>>> >>>> >>> >>> >>> -- >>> ~Craig >>> >> >> > > > -- > ~Craig >-- ~Craig -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140306/079241a9/attachment.html>