User account creation filtered due to spam.

Bug 36587 - Feature: add warning for constructor call with discarded return.
Summary: Feature: add warning for constructor call with discarded return.
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.1.1
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-20 19:21 UTC by Kaz Kylheku
Modified: 2015-07-24 22:29 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Implements -Wunused-objects warning for C++. (955 bytes, patch)
2008-06-20 19:23 UTC, Kaz Kylheku
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kaz Kylheku 2008-06-20 19:21:51 UTC
This patch implements a ``-Wunused-objects'' warning which is triggered if a constructor is called, and the returned object is thrown away.

This can happen by accident when a declaration for an object accidentally omits a name. For instance:

  {
    trace_class ("foo", ...);
    //         ^ missing name
  }

Whereas it is common for C++ function calls to discard returned objects, discarding the result of an explicit constructor call is most likely a mistake.

I developed a patch against GCC 4.1.1 for this (because that's the compiler currently used by my organization). The patch doesn't include a documentation update, just code.
Comment 1 Kaz Kylheku 2008-06-20 19:23:42 UTC
Created attachment 15798 [details]
Implements -Wunused-objects warning for C++.
Comment 2 Kaz Kylheku 2008-06-20 20:26:29 UTC
I should add that this is different from -Wunused-value, because I want this warning emitted even if the constructor (or its corresponding destructor) have side effects.
Comment 3 Jonathan Wakely 2009-12-11 00:37:22 UTC
This would have prevented bugs I've dealt with where critical sections where not protected:
{
  lock_guard (mutex);
  // mutex NOT locked here!
}

But I'm not convinced that doing this is always a mistake. Would the warning be suppressed by casting to void?

  (void) TypeWithSideEffectsInCtor(x);

Comment 4 Kaz Kylheku 2009-12-11 02:34:08 UTC
(In reply to comment #3)
> This would have prevented bugs I've dealt with where critical sections where
> not protected:
> {
>   lock_guard (mutex);
>   // mutex NOT locked here!
> }
> But I'm not convinced that doing this is always a mistake.

Since we don't care about the object, we must care about the constructor side effect. I seem to be under the impression that ISO C++ allows the construction of temporary objects to be optimized away---even if there are side effects in the constructor or destructor! Therefore, it's hard to see a valid use case for this.

 Would the warning be
> suppressed by casting to void?
>   (void) TypeWithSideEffectsInCtor(x);

Not as implemented, I am afraid. The diagnostic is still produced that the object is discarded. This is can be regarded as a flaw; something explicitly requested by a cast should not be diagnosed.
Comment 5 Jonathan Wakely 2009-12-11 10:39:22 UTC
(In reply to comment #4)
> > But I'm not convinced that doing this is always a mistake.
> 
> Since we don't care about the object, we must care about the constructor side
> effect. I seem to be under the impression that ISO C++ allows the construction
> of temporary objects to be optimized away---even if there are side effects in
> the constructor or destructor! Therefore, it's hard to see a valid use case for
> this.

Certain temporaries (such as those created during copying or reference binding) can be optimised away, I don't think it's true of temporaries created explicitly like this.  e.g. I think this should work:

std::ofstream("lockfile");  // creates ./lockfile if it doesn't exist

(assuming write permission in the directory, and ignoring races and other reasons it might be a bad idea)
Comment 6 Kaz Kylheku 2009-12-11 11:57:58 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > > But I'm not convinced that doing this is always a mistake.
> > 
> > Since we don't care about the object, we must care about the constructor side
> > effect. I seem to be under the impression that ISO C++ allows the construction
> > of temporary objects to be optimized away---even if there are side effects in
> > the constructor or destructor! Therefore, it's hard to see a valid use case for
> > this.
> Certain temporaries (such as those created during copying or reference binding)
> can be optimised away, I don't think it's true of temporaries created
> explicitly like this.

I've gone over the relevant 14882:2003 sections and have come to the same conclusion.
Comment 7 Johannes Schaub 2013-09-24 21:23:01 UTC
(In reply to Jonathan Wakely from comment #3)
> This would have prevented bugs I've dealt with where critical sections where
> not protected:
> {
>   lock_guard (mutex);
>   // mutex NOT locked here!
> }
> 

Jonathan, unfortunately that code won't create and discard a temporary object, so I am not sure whether this patch would have caught the mistake of creating a default constructed lock_guard (whatever that means).
Comment 8 Jonathan Wakely 2013-09-24 21:49:42 UTC
(In reply to Johannes Schaub from comment #7)
> Jonathan, unfortunately that code won't create and discard a temporary
> object,

Yes, you're right, there's no temporary so it's not relevant to this PR.

(Although your "whatever that means" assumes my example used std::lock_guard, but if that was the case I'd have needed to use some template arguments.  Other scoped-lock types are DefaultConstructible, e.g. std::unique_lock<T>)
Comment 9 Jonathan Wakely 2013-09-24 21:52:51 UTC
I still think it would be nice to get a warning for e.g.

   std::unique_lock<M>(m);

where "M m" is visible in an enclosing scope and would have been a viable argument for another constructor, but that would be a different enhancement request.
Comment 10 Manuel López-Ibáñez 2015-07-23 13:51:33 UTC
(In reply to Kaz Kylheku from comment #1)
> Created attachment 15798 [details]
> Implements -Wunused-objects warning for C++.

Patches need to be properly tested and submitted. See https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps

The few people that have the power to approve patches are very busy and they very rarely read bugzilla. Patches attached to bugzilla are usually understood as proof-of-concept or work-in-progress, not actual submissions.
Comment 11 Kaz Kylheku 2015-07-24 18:47:07 UTC
(In reply to Manuel López-Ibáñez from comment #10)
> (In reply to Kaz Kylheku from comment #1)
> > Created attachment 15798 [details]
> > Implements -Wunused-objects warning for C++.
> 
> Patches need to be properly tested and submitted. See
> https://gcc.gnu.org/wiki/GettingStarted#Basics:
> _Contributing_to_GCC_in_10_easy_steps
> 
> The few people that have the power to approve patches are very busy and they
> very rarely read bugzilla.

The bug database has an "enhancement" type, so obviously, it is to be used for submitting enhancements. Why would you duplicate effort by implementing a different process for tracking submissions?

In June 2008, when I submitted this, here is how the above Wiki page looked:

https://gcc.gnu.org/wiki/GettingStarted?action=recall&rev=19

There is no mention of any special process at that time.

Please "grandfather" old submissions that were posted to Bugzilla before the special submission process was described in the Wiki.

>Patches attached to bugzilla are usually understood as proof-of-concept or work-in-progress, not actual submissions.

I deployed that change to large team of developers, and the toolchain with that change went to customers also. The warning caught problems that were fixed and didn't appear to break anything.

So yes, actual submission.

Today, I no longer care about upstreaming code to OSS projects because of prima donna attitudes like this. It's just too much effort dealing with the barriers.

In my own projects, I accept good patches, even if they are written on a grease-stained napkin.

If I lead by example, maybe others will follow.
Comment 12 Jonathan Wakely 2015-07-24 21:21:42 UTC
(In reply to Kaz Kylheku from comment #11)
> The bug database has an "enhancement" type, so obviously, it is to be used
> for submitting enhancements.

No, it's for submitting enhancement *requests*, i.e. asking for or suggesting enhancements.

Patches implementing those enhancements should be sent to the gcc-patches list, like all patches.

> Why would you duplicate effort by implementing
> a different process for tracking submissions?

The process for submitting patches has always been to send them to the gcc-patches list for review, why would you duplicate effort by asking reviewers to also track patches in bugzilla?

 
> In June 2008, when I submitted this, here is how the above Wiki page looked:
> 
> https://gcc.gnu.org/wiki/GettingStarted?action=recall&rev=19
> 
> There is no mention of any special process at that time.

There is still no "special process", the process for submitting patches is documented at https://gcc.gnu.org/contribute.html#patches and has always been to send them to the gcc-patches mailing list. That wiki page says nothing about attaching patches to bugzilla, but it does link to https://gcc.gnu.org/contribute.html which describes the patch submission process.

Of course we welcome suggestions for enhancements, especially if they come with patches implementing the suggestion (that's the best kind! :-) but the process for submitting patches is to send them to the mailing list (and there are also legal prerequisites to be met, as described at the link above).

If you're not interested in submitting the patch through that process that's unfortunate. Maybe someone else will be interested enough to do so on your behalf, but that won't happen automatically. There is no way to go through bugzilla and find patches posted here that were never sent via the correct process (there are thousands of attachments in bugzilla, some are testcases, some are patches that don't work, some are patches which were committed after being sent to the mailing list, some are patches that were superseded by improved patches sent to the list ... there is no way to automatically process them and find the ones that were never dealt with).
Comment 13 Manuel López-Ibáñez 2015-07-24 22:29:30 UTC
(In reply to Kaz Kylheku from comment #11)
> I deployed that change to large team of developers, and the toolchain with
> that change went to customers also. The warning caught problems that were
> fixed and didn't appear to break anything.

If the patch were to be upstreamed, it will be reviewed, regression tests would be added to make sure it doesn't silently break, and your customers could update to newer versions of GCC without losing the warning.

> Today, I no longer care about upstreaming code to OSS projects because of
> prima donna attitudes like this. It's just too much effort dealing with the
> barriers.
> 
> In my own projects, I accept good patches, even if they are written on a
> grease-stained napkin.

Perhaps your project is of a size that your team can do this. This is not the case in large free-software projects, which all have much more pending work to do than people to do it.

We already have trouble keeping up with the reviews of the patches people submit following the proper procedure (just subscribe to gcc-patches and try to read all that is going on there), which is supposed to favour quality and future maintainability rather than quantity and quick development.

We do not have enough people with enough time to confirm all the bug reports that are submitted (subscribe to gcc-bugs if you are brave enough). Thus, if something is not critical, you do not actively pursue it and the active developers have something more interesting or urgent to work on, your patch may be ignored, even if you follow the proper procedures. See https://gcc.gnu.org/wiki/Community

(The above is not a statement about whether the current procedures are ideal or even beneficial. It is just a description of the status-quo, which seems to work for the people who contribute patches every day to GCC, even if it doesn't work so well for people that contribute only sporadically).