Page MenuHomePhabricator

C.128 override, virtual keyword handling
ClosedPublic

Authored by kkleine on May 2 2019, 6:32 AM.

Details

Summary

According to [C128] "Virtual functions should specify exactly one
of virtual, override, or final", I've added override where a
virtual function is overriden but the explicit override keyword
was missing. Whenever both virtual and override were specified,
I removed virtual. As C.128 puts it:

[...] writing more than one of these three is both redundant and
a potential source of errors.

I anticipate a discussion about whether or not to add override to
destructors but I went for it because of an example in [ISOCPP1000].
Let me repeat the comment for you here:

Consider this code:

struct Base {
  virtual ~Base(){}
};

struct SubClass : Base {
  ~SubClass() {
    std::cout << "It works!\n";
  }
};

int main() {
  std::unique_ptr<Base> ptr = std::make_unique<SubClass>();
}

If for some odd reason somebody removes the virtual keyword from the
Base struct, the code will no longer print It works!. So adding
override to destructors actively protects us from accidentally
breaking our code at runtime.

[C128]: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final
[ISOCPP1000]: https://github.com/isocpp/CppCoreGuidelines/issues/1000#issuecomment-476951555

Event Timeline

kkleine created this revision.May 2 2019, 6:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 6:32 AM
kwk edited the summary of this revision. (Show Details)May 2 2019, 6:34 AM

Thank you! LGTM, in general we avoid "large" refactoring changes to avoid polluting the blame list but the changes are relatively local and they are good changes that can catch real bugs in the future. I would like a second set of eyes though.

teemperor accepted this revision.May 2 2019, 10:21 AM

LGTM module some minor code style issues (You removed some virtuals but didn't fix the indentation of the parameters on the next line, see the inline comments for examples).

lldb/include/lldb/Utility/Baton.h
67

code style

lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
306

code style

351

code style

This revision is now accepted and ready to land.May 2 2019, 10:21 AM
kwk added a subscriber: kwk.May 2 2019, 12:23 PM

LGTM module some minor code style issues (You removed some virtuals but didn't fix the indentation of the parameters on the next line, see the inline comments for examples).

@teemperor I would have loved to use clang-format for this but apparently the LLDB code is still manually formatted. It would be a bliss to just use the LLVM clang-format style and never ever talk about "style" again.

kwk added a comment.EditedMay 2 2019, 12:30 PM

@teemperor I've addressed your requests for style changes. As it turns out, clang-format did indeed change nothing else in those files. I wonder if I can turn it on everywhere without polluting the history with formatting commits. Thank you for pointing out those style issues. This is one of the first changes I've done to LLDB and I'm very happy that you and @shafik like the change.

In D61440#1488424, @kwk wrote:

LGTM module some minor code style issues (You removed some virtuals but didn't fix the indentation of the parameters on the next line, see the inline comments for examples).

@teemperor I would have loved to use clang-format for this but apparently the LLDB code is still manually formatted. It would be a bliss to just use the LLVM clang-format style and never ever talk about "style" again.

I assume you mean running clang-format manually on each file? If yes, then check out git-clang-format which works just fine in these situations.

teemperor accepted this revision.May 2 2019, 12:39 PM

FYI, I still think there are some style issues there (e.g. the AppleObjCRuntimeV2.cpp change makes it longer than 80 chars) but I can just clang-format this before committing.

Otherwise, unless someone objects to this change I'll merge this tomorrow. Thanks for the patch @kkleine!

kkleine updated this revision to Diff 197848.May 2 2019, 12:48 PM
kwk added a comment.May 2 2019, 12:50 PM

FYI, I still think there are some style issues there (e.g. the AppleObjCRuntimeV2.cpp change makes it longer than 80 chars) but I can just clang-format this before committing.

Otherwise, unless someone objects to this change I'll merge this tomorrow. Thanks for the patch @kkleine!

@teemperor I've formatted the AppleObjCRuntimeV2.cpp using git clang-format and it worked. Thank you for pointing me this handy tool. But as you probably can see now in commit 0b4b24065a12678c773f8c3f0fc722e50405fc85, not all the changes were related to the override keyword.

Not sure how you invoke git clang-format, but I assume you specified the wrong commit range...

I personally use some custom command that executes this git rebase -i --autosquash -x 'git clang-format master && git commit -a --amend --no-edit' master which essentially clang-formats all your git commits (i.e. the ones that are in the local branch but not in master). So if you have the original version of this diff as its own git commit on a separate branch then this command should do all the work for you.

kwk added a comment.May 2 2019, 1:30 PM

Not sure how you invoke git clang-format, but I assume you specified the wrong commit range...

That is correct in this case I've used clang format directly on the whole file. Sorry if that was a noob mistake.

I personally use some custom command that executes this git rebase -i --autosquash -x 'git clang-format master && git commit -a --amend --no-edit' master which essentially clang-formats all your git commits (i.e. the ones that are in the local branch but not in master). So if you have the original version of this diff as its own git commit on a separate branch then this command should do all the work for you.

Thank you for that tip. Unfortunately I've started not on a branch and when I create it now things become complicated. I'm not famiiar enough with the arc tool and will do some reading before just calling arc diff next time. Because when I do that now after rebasing, arc tries to upload a commit that is no longer in my history.

How do you handle rebasing or squashing here? When I run your special command from above, it will rewrite the git history which to me is a no go when I send my code to review. If all these reviews are going to happen on Github anytime, then you would embrace having an append-only history.

You don't have to use arc, you can also just update the code via the "Update diff" button and upload a patch file. So it doesn't really matter how you organize your local git branch as long as you can produce a patch file from the changes. Phabricator takes care of making diffs between the patches you upload, so there is always a change history here.

kkleine updated this revision to Diff 197922.May 3 2019, 12:04 AM
kwk added a comment.May 3 2019, 12:07 AM

@teemperor I've squashed my commits without the last one that did those many changes in the file lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp. I hope at least the result is fine now.

Looks good now, thanks!

kwk added a comment.May 3 2019, 1:25 AM

Looks good now, thanks!

Thank you for being patient with me! Can you merge the change for me please? I don't have commit rights, yet.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 3:02 AM