[PATCH] D126880: [clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[David Friberg via Phabricator via cfe-commits]

dfrib added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp:25
+
+struct RefMember {
+  int &r;

Differentiate between lvalue reference and rvalue reference members using these 
terms instead of "ref" and "refref". E.g.:

- `RefMember` -> `LvalueRefMember`
- `RefRefMember` -> `RvalueRefMember`

(apply to all cases below).



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp:40
+
+struct ConstAndRefMember {
+  int const c;

`ConstAndRefMembers`



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp:100
+};
+
+template 

Should we test for array types also? E.g.:

```
template using Array = int[N];

struct ConstArrayMember {
  const Array<1> c;
};

struct LvalueRefArrayMember {
  Array<2>& lvr;   
};

struct ConstRefArrayMember {
  Array<3> const& clvr;
};

struct LvalueRefArrayMember {
  Array<4>&& rvr;   
};
```



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp:121
+TemplatedRef t3{123};
+TemplatedOk t4{};

Consider expanding with the the non-const lvalue ref case, e.g.

```
TemplatedOk t4{};
TemplateRef t5{t4.t};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126880

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

[David Friberg via Phabricator via cfe-commits]

dfrib added a comment.

In D126818#3935740 , @rjmccall wrote:

> I'm too often slow to actually apply edits to the ABI document.  There's been 
> plenty of time for feedback on this one; go ahead and act like it's accepted.

CWG 2596 was discussed at Kona and, afaict, CWG is opting for a path of least 
effort, with a different result  than 
what is implemented this patch and previously discussed in the ABI issue 
:

> **CWG 2022-11-10**
>
> The friend definitions should conflict with friend definitions from other 
> instantiations of the same class template, consistent with how 
> non-constrained friends would work. Note that the enclosing dependent class 
> type does not appear in the friend function's signature, which is unusual.




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

https://reviews.llvm.org/D126818

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

[David Friberg via Phabricator via cfe-commits]

dfrib added a comment.

In D126818#3941036 , @erichkeane 
wrote:

> In D126818#3940898 , @dfrib wrote:
>
>> In D126818#3935740 , @rjmccall 
>> wrote:
>>
>>> I'm too often slow to actually apply edits to the ABI document.  There's 
>>> been plenty of time for feedback on this one; go ahead and act like it's 
>>> accepted.
>>
>> CWG 2596 was discussed at Kona and, afaict, CWG is opting for a path of 
>> least effort, with a different result 
>>  than what is implemented this patch 
>> and previously discussed in the ABI issue 
>> :
>>
>>> **CWG 2022-11-10**
>>>
>>> The friend definitions **should conflict with friend definitions from other 
>>> instantiations of the same class template**, consistent with how 
>>> non-constrained friends would work. Note that the enclosing dependent class 
>>> type does not appear in the friend function's signature, which is unusual.
>
> Can you clarify the difference here?  What did we choose different?  The 
> example from that CWG issue is exactly in the test for this (see the top of 
> `useS`) so I'm not sure what difference we're missing? Can you clarify what 
> I'm not matching here?

I wrote the CWG issue and the example therein indeed flags exactly what is 
being addressed by this patch, but I'm referring particularly to the comment 
added from CWGs Kona meeting 2022-11-10 (see now **emphasized** quote above). I 
may be mistaken, but I read it as proposing to make `useS` ill-formed, 
emphasizing that you need to include the class somewhere in the hidden friend's 
signature, whereas overloading as shown in the `useS` example would not be 
supported.

For more details, see the Kona minutes 

 at EDG-wiki.


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

https://reviews.llvm.org/D126818

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

[David Friberg via Phabricator via cfe-commits]

dfrib added a comment.

In D126818#3941201 , @erichkeane 
wrote:

> [...] particularly since the suggested wording says the opposite of what I 
> THOUGHT the discussion was doing at the end?
> [...] We might find ourselves wanting to hold off until CWG comes up with 
> actual wording?

Same reflection, and agreed.

> I think perhaps we need to wait on CWG to clarify what they mean, at least by 
> including a wording consistent with that top thing.  The unfortunate part 
> here is that Clang implements 1/2 of this at the moment: we implement the 
> SEMA changes, but not the mangling changes for the current wording.

Should consider asking on the CWG reflector to make sure they are aware of 
Clang's quite far-going implementation experience with fixing this defect (in 
the way originally proposed), and of Itanium C++'s/@rjmccall's view that the 
proposed ABI updates looks reasonable?


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

https://reviews.llvm.org/D126818

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits