Page MenuHomePhabricator

[clang-format] [PR34574] Handle [[nodiscard]] attribute in class declaration
ClosedPublic

Authored by MyDeveloperDay on May 4 2020, 12:22 PM.

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=34574
https://bugs.llvm.org/show_bug.cgi?id=38401

template <typename T>
class [[nodiscard]] result
{
  public:
    result(T&&)
    {
    }
};

formats incorrectly to

template <typename T>
class [[nodiscard]] result{public : result(T &&){}};

Diff Detail

Event Timeline

MyDeveloperDay created this revision.May 4 2020, 12:22 PM

I have an example where this goes wrong in ObjC (modulo indentation):

struct Stuff stuff[] = {                                                                                                                                                                                                                                              
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},                                                                                                                                                                                                                                       
      {key, value},                                                                                                                                                                                                                                   
      {nil, 0}};

gets formatted to (module indentation):

struct Stuff stuff[] = {                                                                                                                                                                                                                                              
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},  // comment                                                                                                                                                                                                                                       
      {key, value},                                                                                                                                                                                                                                       
      {key, value},                                                                                                                                                                                                                                   
      {
        nil, 0
      }
  };

I think we should try harder to detect the beginning of an attribute specifier. I think there may be a helper for that somewhere in clang-format.

krasimir added inline comments.May 5 2020, 9:52 AM
clang/lib/Format/UnwrappedLineParser.cpp
2401

nit: add, e.g., "... or an [[attribute]] ..."

MyDeveloperDay planned changes to this revision.May 5 2020, 12:22 PM

I have an example where this goes wrong in ObjC (modulo indentation):

I think we should try harder to detect the beginning of an attribute specifier. I think there may be a helper for that somewhere in clang-format.

could you give me your .clang-format settings standard LLVM gives me

struct Stuff stuff[] = {{key, value}, // comment
                        {key, value}, // comment
                        {key, value}, // comment
                        {key, value}, {key, value}, {nil, 0}};

I'm not familiar with ObjC, but hope this helps:

% cat ~/example.mm   # left as-is by clang-format without D79354                 
(int)f : (int)arg {
  struct A as[] = {{a11, a12},  //
                   {a21, a22},  //
                   {nil, 0}};
  return 0;
}
% bin/clang-format ~/example.mm  # clang-format with D79354
(int)f : (int)arg {
  struct A as[] = {
    {a11, a12},     //
        {a21, a22}, //
    {
      nil, 0
    }
  };
  return 0;
}
%

Reduce false formatting

MyDeveloperDay marked an inline comment as done.May 5 2020, 2:51 PM
krasimir added inline comments.May 6 2020, 11:33 AM
clang/lib/Format/UnwrappedLineParser.cpp
2404

Just to make sure I follow -- at this point why can't we check for the more specific TT_AttributeSquare instead of the general tok::l_square (if that's available, not sure when it gets computed) (isOneOf accepts both tok::'s and TT_'s).

Ditto for the new tok::l_square below.

MyDeveloperDay planned changes to this revision.May 7 2020, 8:06 AM
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2404

You are quite correct I can use TT_AttributeSquare here... let me change that

Change to TT_AttributeSquare

MyDeveloperDay marked an inline comment as done.May 7 2020, 11:36 AM
krasimir accepted this revision.May 7 2020, 1:02 PM

Thank you! Looks good with a couple of nits.

clang/lib/Format/UnwrappedLineParser.cpp
2402

nit: add a fullstop

2435

slight concern: check if the next token is TT_AttributeSquare and only consume it in that case. Other languages that are using TT_AttributeSquare may have a different syntax for their attributes than [[attribute]], e.g. recently added [[ https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/attributes/ | C# [attributes] ]].

This revision is now accepted and ready to land.May 7 2020, 1:02 PM

Address review comments and ensure existence of closing ]

MyDeveloperDay marked an inline comment as done.May 7 2020, 2:27 PM
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2435

I agree, in fact I had the same concern myself, I kind of assumed because we'd of seen

[[ .... ] then it would be obvious that the next would be ]`

But on reflection I think it could potentially be other things too

MyDeveloperDay marked 2 inline comments as done.May 7 2020, 2:28 PM
krasimir accepted this revision.May 7 2020, 2:48 PM
This revision was automatically updated to reflect the committed changes.