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

Vlad Serebrennikov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 2 11:06:46 PST 2022


Endill 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}}
+}
----------------
aaron.ballman wrote:
> 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).
I think that this one is the case when DR explains it better, because that's what it is about. Adding a comment seems more appropriate for something that's not given enough attention or accent in the whole context of a DR test, as opposed to test code itself.

@shafik Is it fine by you if I don't add any comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139173



More information about the cfe-commits mailing list