clang-format deletes whitespace before ref-qualifier in user-defined conversion type-id

[Quuxplusone]

Bugzilla Link	PR45992
Status	CONFIRMED
Importance	P normal
Reported by	John Freeman (jfreeman08@gmail.com)
Reported on	2020-05-19 08:47:22 -0700
Last modified on	2020-05-19 14:21:45 -0700
Version	10.0
Hardware	PC Linux
CC	djasper@google.com, klimek@google.com, llvm-bugs@lists.llvm.org, mydeveloperday@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

Compare two consecutive commits:

• parent: https://github.com/ripple/rippled/commit/97712107b71a8e2089d2e3fcef9ebf5362951110
• child: https://github.com/ripple/rippled/commit/fdf6b09433cb4ca9872b89ed3419e0f594523442

The file src/test/unit_test/SuiteJournal.h is unchanged between them. They both had checks that ran clang-format on that file. You can find the logs of the checks by clicking the red x or green check in the top left before the commit message, then clicking "Details" for "clang-format / check". I'll link them here:

• parent: https://github.com/ripple/rippled/runs/647802934
• child: https://github.com/ripple/rippled/runs/687680298

They ended up running two slightly different versions of clang-format available on different days through the official PPA (check the "Install clang-format" step):

• parent: 10.0.1~++20200504072809+8e7ae355ba9-1~exp1~20200504054307.155
• child: 10.0.1~++20200518104422+eaae6dfc545-1~exp1~20200518085019.161

The latter version reformatted the file src/test/unit_test/SuiteJournal.h while the earlier did not:

--- a/src/test/unit_test/SuiteJournal.h
+++ b/src/test/unit_test/SuiteJournal.h
@@ -98,7 +98,7 @@ public:
         : sink_(partition, threshold, suite), journal_(sink_)
     {
     }
-    operator beast::Journal &()
+    operator beast::Journal&()
     {
         return journal_;
     }

I'm not sure if this was a bug fix or a bug introduction. I'm calling it a bug only because from my perspective, I am getting non-determinism even when following the official install instructions for one major version of clang-format, and even when those instructions installed the same patch version, if I'm reading it correctly. If there is another way we can pin an even more specific version of clang-format to install, or otherwise guarantee consistent formatting day-to-day, please let me know.

[Quuxplusone]

I think the two versions reference commit hashes in their version strings, and
in between those commits lie these two which affected clang-format, and likely
in this case:

- https://github.com/llvm/llvm-
project/commit/7ae6db9cf0c0b11a6f753cb1cf8dabb512e70f6f
- https://github.com/llvm/llvm-
project/commit/baeb500a8ca52b144d1bc14a04130964d68eaade

The second commit references this issue, which also concerns whitespace in user-
defined conversion operators: https://bugs.llvm.org/show_bug.cgi?id=45357

[Quuxplusone]

If I run test this again 11.0 (last Windows Snapshot 3rd Feb)

I get (as you do)

class Foo()
{
    bool operator A&();
    bool operator Foo::A &();

    bool operator A&()
    {
        // ...
    }

    bool operator Foo::A &()
    {
        // ...
    }
}

If I run it using the latest trunk I get the following

class Foo()
{
    bool operator A&();
    bool operator Foo::A&();

    bool operator A&()
    {
        // ...
    }

    bool operator Foo::A&()
    {
        // ...
    }
}

Are you using clang-format 10? or trunk?

I know the changes were at least partially backported to 10

Paul

[Quuxplusone]

I'm running version 10. I put the version numbers as apt knows them in the OP, but I'll copy them here:

• parent: 10.0.1~++20200504072809+8e7ae355ba9-1~exp1~20200504054307.155
• child: 10.0.1~++20200518104422+eaae6dfc545-1~exp1~20200518085019.161

Here are the steps that my check jobs take to install clang-format-10, which can be found in the GitHub Workflow (https://github.com/ripple/rippled/blob/develop/.github/workflows/clang-format.yml):

sudo tee /etc/apt/sources.list.d/llvm.list >/dev/null <<EOF
deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-10 main
deb-src http://apt.llvm.org/bionic/ llvm-toolchain-bionic-10 main
EOF
wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add
sudo apt-get update
sudo apt-get install clang-format-10

These steps on May 5 installed 10.0.1~++20200504072809+8e7ae355ba9-1~exp1~20200504054307.155, and on May 18 installed 10.0.1~++20200518104422+eaae6dfc545-1~exp1~20200518085019.161.

[Quuxplusone]

I've built the tip of the 10 branch and it shows

class Foo() {
  bool operator A &();
  bool operator Foo::A &();

  bool operator A &() {
    // ...
  }

  bool operator Foo::A &() {
    // ...
  }
}

which I think is what you are seeing

[Quuxplusone]

Actually I made a mistake because my .clang-format file didn't have PointerAlignment: Left (as yours does)

[Quuxplusone]

Checking your code, formatting with clang-format 10 gives the following

class SuiteJournal
{
    SuiteJournalSink sink_;
    beast::Journal journal_;

public:
    SuiteJournal(
        std::string const& partition,
        beast::unit_test::suite& suite,
        beast::severities::Severity threshold = beast::severities::kFatal)
        : sink_(partition, threshold, suite), journal_(sink_)
    {
    }
    operator beast::Journal&()
    {
        return journal_;
    }
};

To be honest I think this is correct. You have

PointerAlignment: Left

So I feel this change is because clang-format is now doing the correct thing,
please could you confirm.