[PATCH] D139173: [clang] Add test for CWG600

Fri Dec 2 10:57:38 PST 2022

aaron.ballman added inline comments.

================
Comment at: clang/test/CXX/drs/dr6xx.cpp:18
+  sp->f(2);
+  sp->f(2.2); // expected-error {{is a private member}}
+}
----------------
Endill wrote:
> Endill wrote:
> > shafik wrote:
> > > Maybe add a comment above this saying something like:
> > > 
> > > ```
> > > // access control is applied after overload resolution
> > > // [class.access.general]p4 "For an overload set, access control is applied only to the function selected by overload resolution."
> > > ```
> > I tend to like the idea, but I wonder about general rule for adding such explanations. Currently DR tests contain very little of those. 
> > 
> > If we're going to add explanations, we should also decide whether we're going to cite the standard, or paraphrase (and/or) explain intent. My concern is that both references to standard and citations could grow old relatively quickly, and we don't have any tools to help, at least yet.
> @aaron.ballman What do you think?
We don't typically add a significant amount of comments to test files unless what is being tested needs some explanation. So, IMO, if the DR test case is "tricky" in some way and we're trying to demonstrate that we're testing a specific sentence or two from the standard, then I think a comment with standards wording is reasonable (please don't just cite `[foo.bar]p12` though -- add the standards wording!). However, if the test is pretty straightforward, or the DR explains in more detail what's going on, then I don't think we need to add the comment (the references get stale rather quickly, even with stable names as in C++).

All that said, if you're on the fence about whether to add a comment or not, go ahead and add it (IMO).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139173