[PATCH] D69764: [clang-format] Add Left/Right Const fixer capability

Stephen Kelly via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 21 15:43:55 PDT 2020


steveire added a comment.

I like the approach of using clang-format to implement this. It's much faster than a `clang-tidy` approach.

The broader C++ community has already chosen `East`/`West` and it has momentum. If you choose `Left`/`Right` now, you will get pressure to add `East`/`West` in the future, which means we'll have the synonyms we want to avoid.

The broader C++ community already has understanding of `East`/`West`. Trying to change that now should be out of scope for this patch. This patch should use `East`/`West`.

I ran this on a large codebase and discovered some problems with this patch. Given this `.clang-format` file:

  BasedOnStyle: LLVM
  PointerAlignment: Left
  ConstStyle: Right

I get the failures as commented here:

  template <typename T> struct Foo {};
  
  template <> struct Foo<int> {
    static const int bat;
    static const int fn();
  };
  
  // 'const' inserted in wrong place:
  int const Foo<int>::bat = 0;
  // 'const const' inserted in wrong place:
  int const Foo<int>::fn() {
      return 0;
  }
  
  void foo() {
    Foo<Foo<int>> ffi;
    // '* const' inserted instead of 'const':
    const Foo<Foo<int>>* p = const_cast<const Foo<Foo<int>>*>(&ffi);
  }
  
  // 'const >' inserted instead of 'const':
  Foo<const Foo<int>> P;
  
  template <typename T>
  // 'const const' inserted instead of 'const':
  void fn(const Foo<T>& i);
  
  // This is needed to trigger the above 'const const' bug above:
  #if 0
  #else
  #endif
  
  namespace ns {
  struct S {};
  } // namespace ns
  
  // not modified:
  void fns(const ns::S& s);

Some of these (eg the `int Foo<int>::bat const = 0`) are the kinds of breakages `clang-format` so far avoids by not reordering tokens, as @sammccall was saying. I don't know if the `clang-format` parser is smart enough to not cause those bugs.

Thanks for working on this!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69764/new/

https://reviews.llvm.org/D69764





More information about the cfe-commits mailing list