[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

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