lock3 / meta

122 stars 11 forks source link

Clang format #256

Open alexgeek opened 4 years ago

alexgeek commented 4 years ago

Hi,

Just wondering if it's possible to configure clang format properly when using the metaprogramming syntax? I imagine the answer is not at this stage.

The issue:

consteval void gen_member(meta::info member) {
  -> fragment struct {
    void unqualid("set_", %{meta::name_of(member)})(const typename(%{meta::type_of(member)})& unqualid(%{meta::name_of(member)})) {
      this->unqualid(%{meta::name_of(member)}) = unqualid(%{meta::name_of(member)});
    }
  };

  -> fragment struct {
    typename(%{meta::type_of(member)}) unqualid("get_", %{meta::name_of(member)})() {
      return unqualid(%{meta::name_of(member)});
    }
  };
}

Is formatted to:

consteval void gen_member(meta::info member) {
  ->fragment struct {
    void unqualid("set_", % {meta::name_of(member)})(const typename(% {meta::type_of(member)}) & unqualid(% {meta::name_of(member)})) {
      this->unqualid(% {meta::name_of(member)}) = unqualid(% {meta::name_of(member)});
    }
  };

  ->fragment struct {
    typename(% {meta::type_of(member)}) unqualid("get_", % {meta::name_of(member)})() {
      return unqualid(% {meta::name_of(member)});
    }
  };
}

Which looks bad and changes the meaning of the program (second doesn't compile).

Thanks, Alex

alexgeek commented 4 years ago

This seems to prevent adding the space after the % symbol:

bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
                                         const FormatToken &Right) {
  const FormatToken &Left = *Right.Previous;
  if (Right.Tok.getIdentifierInfo() && Left.Tok.getIdentifierInfo())
    return true; // Never ever merge two identifiers.
  if (Style.isCpp()) {
    if (Left.is(tok::kw_operator))
      return Right.is(tok::coloncolon);
    if (Right.is(tok::l_brace) && Right.Previous->is(tok::percent)) // <-- here
      return false;
    if (Right.is(tok::l_brace) && Right.is(BK_BracedInit) &&
        !Left.opensScope() && Style.SpaceBeforeCpp11BracedList)
      return true;
  } ...

I would have thought a check for just 'percentl_brace' would work but it didn't.

I tried adding the following too but it didn't fix removing the space after the arrow ('->'):

if (Right.is(tok::kw_fragment) || Right.is(tok::kw___fragment))
      return true;
DarkArc commented 4 years ago

I looked into this... On the surface, the issue is that clang-format isn't lexing with -freflection. I could add an -freflection flag or configuration option, to enable clang-format to use the reflection lexing. However, that seems to be unprecedented, at least looking at the documentation.

You could apply this as a patch to change your build to always be in -freflection mode:

diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 88750386b136..df5cda3f69d1 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -145,7 +145,7 @@ LANGOPT(GNUAsm            , 1, 1, "GNU-style inline assembly")
 LANGOPT(Coroutines        , 1, 0, "C++20 coroutines")
 LANGOPT(DllExportInlines  , 1, 1, "dllexported classes dllexport inline methods")
 LANGOPT(RelaxedTemplateTemplateArgs, 1, 0, "C++17 relaxed matching of template template arguments")
-LANGOPT(Reflection        , 1, 0, "C++ reflection and metaclasses")
+LANGOPT(Reflection        , 1, 1, "C++ reflection and metaclasses")

 LANGOPT(DoubleSquareBracketAttributes, 1, 0, "'[[]]' attributes extension for all language standard modes")

diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 0377c353210b..c6d2122ff7b5 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -2815,9 +2815,6 @@ static void ParseLangArgs(LangOptions &Opts, ArgList &Args, InputKind IK,
                                    OPT_fno_dollars_in_identifiers,
                                    Opts.DollarIdents);

-  // [Meta] Determine if C++ reflection is supported.
-  Opts.Reflection = Args.hasArg(OPT_freflection);
-
   Opts.PascalStrings = Args.hasArg(OPT_fpascal_strings);
   Opts.setVtorDispMode(
       MSVtorDispMode(getLastArgIntValue(Args, OPT_vtordisp_mode_EQ, 1, Diags)));

This still isn't great though, as you end up with things like:

consteval void gen_member(meta::info member) {
  -> fragment struct {
    void unqualid("set_", %{meta::name_of(member)})(const typename(%{meta::type_of(member)})& unqualid(%{meta::name_of(member)
})) {
      this->unqualid(%{meta::name_of(member)}) = unqualid(%{meta::name_of(member)
});
}
}
;

->fragment struct {
    typename(%{meta::type_of(member)}) unqualid("get_", %{meta::name_of(member)
})() {
      return unqualid(%{meta::name_of(member)});
}
}
;
}

These are secondary issues, probably related to clang format not understanding fragments. I think these problems are out of scope for us right now, given limited resources, so, there likely won't be further movement here/this won't be fixed (by us).

I'll leave this open for any further feedback, or community input; that said, if someone (perhaps yourself) comes up with a great patch that gets clang format working well, I think we could consider pulling that in -- no promises though, everything we add is something we have to maintain (or at least bump into when we sync with upstream -- which with a diff of 473 files changed, 47840 insertions(+), 2803 deletions(-) in feature/metaprogramming against upstream, that's not always trivial) :).