cor3ntin marked 7 inline comments as done.
cor3ntin added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:3775
bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
- bool ConsiderRequiresClauses = true);
+ bool UseOverrideRules = false);
----------------
aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > aaron.ballman wrote:
> > > > > I think we should add some comments here explaining what
> > > > > `UseOverrideRules` means. The old parameter was kind of mysterious as
> > > > > well, but this one feels even more mysterious to me.
> > > > Maybe we should document the whole function, but for that I'd need to
> > > > better understand `UseMemberUsingDeclRules`
> > > >
> > > > The other solution might be to have an `IsOverride` function such that
> > > > both `IsOverride` and `IsOverload` function would dispatch to the same
> > > > internal function.
> > > >
> > > > The other solution might be to have an IsOverride function such that
> > > > both IsOverride and IsOverload function would dispatch to the same
> > > > internal function.
> > >
> > > I think that's a clean solution.
> > I added `IsOverride`
> Oh, I was thinking of something different than what you did. Can we do:
> ```
> bool IsOverload(FunctionDecl *New, FunctionDecl *Old,
> bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs =
> true);
> bool IsOverride(FunctionDecl *MD, FunctionDecl *BaseMD);
> ```
> in the header file, and then in the implementation we dispatch to the same
> static helper function?
>
> (What I was hoping to get rid of was `UseOverrideRules` from the public
> signature entirely so callers don't have to wonder what that means.)
Done!
================
Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:29-30
+ S(this auto); // expected-error {{an explicit object parameter cannot
appear in a constructor}}
+ ~S(this S) {} // expected-error {{an explicit object parameter cannot
appear in a destructor}} \
+ // expected-error {{destructor cannot have any parameters}}
+};
----------------
aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > aaron.ballman wrote:
> > > > > If possible, it would be nicer if we only issued one warning as the
> > > > > root cause is the same for both diagnostics.
> > > > Should we simply remove the newly added diagnostic then?
> > > I think the new diagnostic is helpful; I'm more wondering if we can emit
> > > just the new diagnostic and skip `destructor cannot have any parameters`
> > > if we already said it can't have an explicit object parameter?
> > FYI i remember looking at that and i think it was non trivial, I'd rather
> > punt it
> Okay, when we land this, can you file an issue so we don't lose track of the
> improvement?
Sure!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140828/new/
https://reviews.llvm.org/D140828
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
