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.
Created attachment 15798 [details] Implements -Wunused-objects warning for C++.
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.
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);
(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.
(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)
(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.
(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).
(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>)
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.
(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.
(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.
(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).
(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).
C++17 adds nodiscard attribute which can be placed on class/struct types, functions, constructors and others and then you get a warning if you ignore the value. In the case of struct/class types and constructors that will warn when a temporary value is ignored. Exactly in the case you were asking for a warning. Which was added to GCC by r7-377-gb632761d2c6a65 (and fixes afterwards). So closing as fixed.
(In reply to Manuel López-Ibáñez from comment #13) > (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. In April 2020 I created a patch for the GNU C Preprocessor, with documentation, test cases and everything. I submitted it to the GCC Patches mailing list, exactly as comments in this ticket from 2015 advised me I should have done for this item back in 2008. I received absolutely no replies; not even to reject the submission. I "pinged" it a number of times, to no avail. The four year anniversary is coming up; I'm going to ping again. Then if I'm still ignored, one last time in April 2025.
(In reply to Andrew Pinski from comment #14) > C++17 adds nodiscard attribute which can be placed on class/struct types, > functions, constructors and others and then you get a warning if you ignore > the value. In the case of struct/class types and constructors that will warn > when a temporary value is ignored. Exactly in the case you were asking for a > warning. > > Which was added to GCC by r7-377-gb632761d2c6a65 (and fixes afterwards). > > So closing as fixed. I'm afraid that this doesn't seem like a good resolution. The feature you are referring to is opt-in, per class, whereas the proposed warning imposes the behavior for every class. This is a big difference. The number of situations in which "classname(arg);" as an expression statement with a discarded value is correct is pretty small. This is almost always going to be a coding mistake. Where it isn't a coding mistake, the intent can be indicated using "(void) classname(arg);". The good news is that, at least it would seem that the implementation of the warning can be piggybacked on the nodiscard attribute implementation. The simplest possible requirement is that the option makes the compiler pretend that the attribute has been asserted for every class. (I say tentatively, without having studied the attribute's semantics in detail.) nodiscard could be "stronger" in that if it is asserted, then even the cast to (void) won't silence the diagnostic, so that it's still meaningful to use in spite of the warning option.
No, the nodiscard warnings must be silenced with a cast to void. They can't be "stronger" than that.
(In reply to Kaz Kylheku from comment #15) > In April 2020 I created a patch for the GNU C Preprocessor, with > documentation, test cases and everything. I submitted it to the GCC Patches > mailing list, exactly as comments in this ticket from 2015 advised me I > should have done for this item back in 2008. > > I received absolutely no replies; not even to reject the submission. > > I "pinged" it a number of times, to no avail. > > The four year anniversary is coming up; I'm going to ping again. Then if I'm > still ignored, one last time in April 2025. This one? https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593460.html Was there an earlier submission?
(In reply to Jonathan Wakely from comment #18) > Was there an earlier submission? No there wasn't; that's my mistake in my comment.