Warning for improperly deleted copy constructor?

[Avi Drissman]

https://chromium-review.googlesource.com/c/chromium/src/+/3869741 fixes a copy/paste error:

class MediaWatchTimeChangedDelegate : public WebContentsDelegate {
 public:
  [...]

  MediaWatchTimeChangedDelegate(const LoadStateWaiter&) = delete;
  MediaWatchTimeChangedDelegate& operator=(
      const MediaWatchTimeChangedDelegate&) = delete;

I'd argue that the = delete here is detectably wrong. This isn't a case where the code is deleting a compiler provided function, nor is it deleting a member function that would otherwise be callable (like deleting the long version of a constructor taking a long long). What's happening here is that a converting constructor is being declared out of the blue as deleted. I can't think of a reasonable situation in which this code is what's wanted, and making a copy/paste error breaks the intent of the author.

-Wall doesn't catch this. Should it?

Avi

[Peter Kasting]

It seems like a good error to catch IMO.  Maybe file a bug against the Lexan team?

PK 

[Avi Drissman]

Where does Lexan take bugs?

[Peter Kasting]

On Tue, Sep 6, 2022 at 10:54 AM Avi Drissman <a...@google.com> wrote:

Where does Lexan take bugs?

Looks like crbug.com under "Tools>LLVM", from the "new" bug link on https://g3doc.corp.google.com/company/teams/lexan/index.md?cl=head (found via moma search for "Lexan bug").

PK 

[Avi Drissman]

Thanks.

[Avi Drissman]

https://crbug.com/1360496

[dan...@chromium.org]

I can think of one: where you want to prevent construction from X that is convertible from Y but allow construction from Y. I think this would prevent the conversion because the deleted overload matches?

Foo(const X&) = delete;

Foo(const Y&);

class X { X(const Y&); };

 

-Wall doesn't catch this. Should it?

Avi

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAYwhMRDD%2BmddJPWJcZQ1J7wXDps2-XQcGfjZ0Db_ANkXQ%40mail.gmail.com.

[Avi Drissman]

This fails my rephrasing, "It should be a warning when a deleted function declaration is used for a function call that would already have been ill-formed without it." In your example, I suppose that you'd have to parse the converting constructor of X to verify that the deleted function is actually doing something. 

While this strikes me as being a theoretically warnable case, I don't know enough about the parsing of C++ to know if this is a practically warnable case. I'll leave that to the Lexan folks.

Avi

[dan...@chromium.org]

On Tue, Sep 6, 2022 at 2:41 PM Avi Drissman <a...@google.com> wrote:

This fails my rephrasing, "It should be a warning when a deleted function declaration is used for a function call that would already have been ill-formed without it." In your example, I suppose that you'd have to parse the converting constructor of X to verify that the deleted function is actually doing something. 

While this strikes me as being a theoretically warnable case, I don't know enough about the parsing of C++ to know if this is a practically warnable case. I'll leave that to the Lexan folks.

X could be forward declared in which case you don't know what it can be converted from. And Y could provide operator X() so you would need Y's declaration too.

You could just say you don't want to let this type of C++ exist, and it's not up to Foo if it can control conversions when it's called. Or you could make Foo template the function instead of overload with delete, which also effectively prevents all conversions. It's all a bit awkward cuz that's how things are in this space.

If you required all constructors that aren't legit copy constructors to be marked explicit, then the delete could be required explicit and then you'd notice as an author maybe?

[Daniel Cheng]

It's legal to mark pretty much anything as deleted. Maybe it's possible to somehow generate a heuristic narrow enough to target the "copy paste error turns deleted copy constructor into deleted conversion constructor", but I think it'd be quite difficult and would either never fire or would fire on legitimate cases.

https://abseil.io/tips/142 does encourage explicit, even for multi-arg constructors, but there are some reasonable use cases of converting constructors (the linked ToTW notes one such example).

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaRj9tsOPLQ6sBFKnqgYoCqg4uxDRnxepEc8F7LNgAudig%40mail.gmail.com.