Page MenuHomePhabricator

[clang-format] [PR45357] Fix issue found with operator spacing
ClosedPublic

Authored by MyDeveloperDay on Sun, Apr 26, 9:03 AM.

Details

Summary

This is a tentative fix for https://bugs.llvm.org/show_bug.cgi?id=45357

Spaces seem to be introduced between * and * due to changes brought in for D69573: [clang-format] [PR36294] AlwaysBreakAfterReturnType works incorrectly for some operator functions

We may need to gather some more use cases.

Pre the changes in D69573 the output was:

class UniquePtrGetterAddRefs {
  operator void **() { return reinterpret_cast<void **>(&mPtrStorage); }
}

class ReturnToGlobal {
  operator LocalRef<Object> *() { return &objRef; }
}

In the last windows snapshot (11.0.0.2263) (Feb 2020) the output was:

class UniquePtrGetterAddRefs {
  operator void * *() { return reinterpret_cast<void **>(&mPtrStorage); }
}

class ReturnToGlobal {
  operator LocalRef<Object> *() { return &objRef; }
}

post fix the out is:

class UniquePtrGetterAddRefs {
  operator void **() { return reinterpret_cast<void **>(&mPtrStorage); }
}

class ReturnToGlobal {
  operator LocalRef<Object>*() { return &objRef; }
}

Is the removal of the space between the <Object> and the* what you expect?

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Sun, Apr 26, 9:03 AM

More test cases

MyDeveloperDay edited the summary of this revision. (Show Details)Sun, Apr 26, 9:23 AM
MyDeveloperDay edited the summary of this revision. (Show Details)

I think we are on the right track with this but what if we are dealing with tok::amp in a context similar to operator const FallibleTArray<E>&(); I can speculate that the outcome will be operator const FallibleTArray<E> &(); which is wrong.

MyDeveloperDay marked an inline comment as done.Mon, Apr 27, 12:33 AM
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
2820

I'm not happy about this it feels way too general.

Abpostelnicu requested changes to this revision.Mon, Apr 27, 1:15 AM
Abpostelnicu added a reviewer: Abpostelnicu.
This revision now requires changes to proceed.Mon, Apr 27, 1:15 AM
MyDeveloperDay added a comment.EditedMon, Apr 27, 1:55 AM

@sylvestre.ledru

I'm taking a quick look at formatting the original bug in gecko and whilst the last windows snapshot (Feb 2020) shows the bug

$ clang-format --version
clang-format version 11.0.0

nsTArray.h:939:29: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const nsTArray<E>&() {
                            ^
nsTArray.h:946:35: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const FallibleTArray<E>&() {
                                  ^
nsTArray.h:1147:59: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray_Impl<E, Allocator>&() const& {
                                                          ^
nsTArray.h:1151:43: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray<E>&() const& {
                                          ^
nsTArray.h:1154:49: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const FallibleTArray<E>&() const& {

The current trunk does not

$ clang-format --version
clang-format version 11.0.0 (https://github.com/llvm/llvm-project 1956a8a7cb79e94dbe073e36eba2d6b003f91046)

clang-format -n nsTArray.h

Is Mozilla toolchain using LLVM from the v10 branch?

I think this is fixed by D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107)

@sylvestre.ledru

I'm taking a quick look at formatting the original bug in gecko and whilst the last windows snapshot (Feb 2020) shows the bug

$ clang-format --version
clang-format version 11.0.0

nsTArray.h:939:29: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const nsTArray<E>&() {
                            ^
nsTArray.h:946:35: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const FallibleTArray<E>&() {
                                  ^
nsTArray.h:1147:59: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray_Impl<E, Allocator>&() const& {
                                                          ^
nsTArray.h:1151:43: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray<E>&() const& {
                                          ^
nsTArray.h:1154:49: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const FallibleTArray<E>&() const& {

The current trunk does not

$ clang-format --version
clang-format version 11.0.0 (https://github.com/llvm/llvm-project 1956a8a7cb79e94dbe073e36eba2d6b003f91046)

clang-format -n nsTArray.h

Is Mozilla toolchain using LLVM from the v10 branch?

I think this is fixed by D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107)

Yes, we are using the clang tooling 10.

So for the Mozilla case mentioned in the bug report we either need to ask @hans if D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107) can go into the v10 branch, or perhaps they can apply that path rather than the revert patch

However I believe for @sylvestre.ledru bug ( is somewhat different and needs additional changes)

I've cherry-picked D76850 to 10.x and see if this fixes the issue.

MyDeveloperDay updated this revision to Diff 260261.EditedMon, Apr 27, 3:09 AM

@sylvestre.ledru , @Abpostelnicu

I believe to fix your original request you need a combination of D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107) and this fix (this fix will handle the * * issue in gecko-dev/ipc/mscom/Ptr.h

I've run this new binary over gecko-dev/ipc/mscom and gecko-dev/xpcom/ds and it shows no clang-format warnings

I hope this helps

Need to skip over the template or we won't see this as needing to break on the return type

hans added a subscriber: tstellar.Mon, Apr 27, 4:44 AM

So for the Mozilla case mentioned in the bug report we either need to ask @hans if D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107) can go into the v10 branch, or perhaps they can apply that path rather than the revert patch

Forwarding to @tstellar who manages the 10.0.1 release.

@sylvestre.ledru , @Abpostelnicu

I believe to fix your original request you need a combination of D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107) and this fix (this fix will handle the * * issue in gecko-dev/ipc/mscom/Ptr.h

I've run this new binary over gecko-dev/ipc/mscom and gecko-dev/xpcom/ds and it shows no clang-format warnings

I hope this helps

Yes, definitely you are right D76850, only partially fixes the issue introduced by D69573, and your fix it's a good add-on in order to have a complete fix for the regression.

Just to clarify something here, for me the patch looks good but until I will accept the revision I want to test it on the mozilla codebase.

Abpostelnicu accepted this revision.Tue, Apr 28, 2:38 AM
This revision is now accepted and ready to land.Tue, Apr 28, 2:38 AM
This revision was automatically updated to reflect the committed changes.