ayzhao marked 4 inline comments as done.
ayzhao added inline comments.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:5592
+bool Parser::isConstructorDeclarator(bool IsUnqualified, bool DeductionGuide,
+ bool IsFriend) {
TentativeParsingAction TPA(*this);
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > erichkeane wrote:
> > > > aaron.ballman wrote:
> > > > > shafik wrote:
> > > > > > Instead of adding yet another `bool` flag maybe we can consider
> > > > > > using something like `enum isFriend : bool {No, Yes}`.
> > > > > >
> > > > > > I am sure @aaron.ballman will want to chime in here as well.
> > > > > Heh, so this is where I get worried about the scalability of using
> > > > > enums for these. We really want to use three different enums here,
> > > > > but do we really want to *add* three different enums? I'm unconvinced.
> > > > >
> > > > > However, if we can come up with some template magic to allow for
> > > > > named bool parameters as a generic interface, that would be valuable
> > > > > to use.
> > > > I prefer enums over bools TBH, even if we end up with a million of then
> > > > somewhere.
> > > >
> > > > That said, what about:
> > > >
> > > > https://godbolt.org/z/Kz6jdjobj
> > > >
> > > > ```
> > > > template<typename SpecificThing>
> > > > class Is {
> > > > Is(bool v) : value(v){}
> > > > public:
> > > > bool value;
> > > > static const Is Yes() { return Is{true};}
> > > > static const Is No() { return Is{false};}
> > > >
> > > > operator bool() { return value; }
> > > > };
> > > >
> > > > class Friend{}; // #1
> > > >
> > > > void foo(Is<Friend> f) {
> > > > if (f) {
> > > > ///...
> > > > }
> > > > }
> > > >
> > > > void baz() {
> > > > foo(Is<Friend>::Yes());
> > > > }
> > > > ```
> > > >
> > > > Adding a 'new' thing is as simple as just adding #1 for anything we
> > > > care about. We might want to put them in a namespace of some sort, but
> > > > perhaps not awful?
> > > Yeah, this is along the lines of what I was thinking of! However, I'm
> > > still concerned about that approach because it involves adding a new type
> > > for every situation we have a bool. Empty classes to use as a tag
> > > definitely works, but I was hoping we could use a string literal rather
> > > than a tag type so that we don't have the extra compile time overhead of
> > > adding hundreds of new empty classes. e.g.,
> > > ```
> > > void foo(Is<"Friend"> f) {
> > > if (f) {
> > > // ...
> > > }
> > > }
> > >
> > > void baz() {
> > > foo(Is<"Friend">::Yes); // Yay
> > > foo(Is<"Enemy">::Yes); // Error for type mismatch with Is<"Friend">
> > > }
> > > ```
> > > However, that might require compiling with C++20 (I don't recall), so it
> > > may not be a viable idea.
> > Yeah, referring to stringliterals is troublesome in C++17. However, we
> > COULD do that like:
> >
> > ```
> > void foo(Is<"Friend"_Is> f) {
> > if (f) {
> > // ...
> > }
> > }
> >
> > void baz() {
> > foo(Is<"Friend"_Is>::Yes); // Yay
> > foo(Is<"Enemy"_Is>::Yes); // Error for type mismatch with Is<"Friend">
> > }```
> >
> > by making operator _Is return an integer_sequence.
> That's a really neat idea! If you want to work it up into something that
> could be plausible to add to ADT, I think it's worth an RFC to add the
> interface. I'm guessing the diagnostic behavior of that would be kind of
> gross, but once we move to C++20 we'd be able to use string literal template
> arguments directly and get better diagnostic behavior. The critical part is
> that the code is readable and we get diagnostics when passing an argument to
> the wrong parameter.
I added a `FriendSpecified` enum for now.
I'm going to mark the comment chain as done so that they don't unnecessarily
block the review.
I agree with the idea of a generic type to represent boolean arguments, but I
think it would be out of scope of this patch and that Discourse would be a more
appropriate place to discuss this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53847/new/
https://reviews.llvm.org/D53847
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
