shafik 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:
> 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?
I would be satisfied with this comment alone, it would avoid the concern about
wording and paragraph numbering shifting around in subsequent drafts:
```
// access control is applied after overload resolution
```
While the DR does explain the issue it is not obvious what the resolution is
without spending some time digging, but at least with the comment you know what
your looking for if you want to do the digging.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139173/new/
https://reviews.llvm.org/D139173
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
