Chandler Carruth via llvm-dev
2016-Sep-04 22:40 UTC
[llvm-dev] Adding [[nodiscard]] to Compiler.h
My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD. For compilers that support it, it should be a strict superset of features and functionality. The standard feature was written directly based on the clang warn_unused_result stuff. I would just migrate us onto the spelling and usage pattern that got standardized. All we have to lose are warnings from compilers other than Clang until they implement this feature. That seems like not a huge loss to me honestly. -Chandler On Sun, Sep 4, 2016 at 2:57 PM Sanjoy Das via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hi Justin, > > Justin Bogner wrote: > > Sanjoy Das<sanjoy at playingwithpointers.com> writes: > >> Hi Justin, > >> > >> This SGTM generally, but please make the difference between > >> LLVM_NODISCARD and LLVM_UNUSED_RESULT clear in the code. :) > > > > Right, this is where it gets a little weird. LLVM_NODISCARD would be for > > types, whereas LLVM_UNUSED_RESULT would be for functions. Depending on > > your host compiler, using the wrong one might DTRT, but it won't across > > all compilers. > > > > Do you think documenting this is sufficient, or should we try to name > > these to better represent where they should be used? > > Firstly, perhaps "LLVM_NODISCARD_TYPE" would be a better name? > > Secondly, if we define the following when the host compiler is clang, > > #define LLVM_UNUSED_RESULT __attribute__((warn_unused_result)) > #define LLVM_NODISCARD_TYPE [[clang::warn_unused_result]] > > via some shallow manual testing, it looks like using > LLVM_NODISCARD_TYPE on a function should break the build (though I'd > love it if someone more familiar with clang chimed in on this). So, is > the problem that we can accidentally use LLVM_UNUSED_RESULT on a type > and not know it? > > I think there is a dirty trick here -- we could: > > #define LLVM_UNUSED_RESULT __attribute__((warn_unused_result, > enable_if(true, ""))) > #define LLVM_NODISCARD_TYPE [[clang::warn_unused_result]] > > This breaks the (clang) build if LLVM_UNUSED_RESULT is used on a type. > > It would still be possible to not do the right thing on GCC or MSVC > and be able to build a binary, but I think as long as the clang bots > catch bad uses we're okay. > > -- Sanjoy` > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160904/f8df9cbf/attachment.html>
Sanjoy Das via llvm-dev
2016-Sep-04 22:53 UTC
[llvm-dev] Adding [[nodiscard]] to Compiler.h
Hi Chandler, Chandler Carruth wrote: > My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD. > > For compilers that support it, it should be a strict superset of features and functionality. The standard feature was > written directly based on the clang warn_unused_result stuff. Maybe I misunderstood something, but with clang 3.8.1 I cannot write: int [[clang::nodiscard]] f(); so I'm not sure if LLVM_NODISCARD provides a superset of the functionality. -- Sanjoy
Chandler Carruth via llvm-dev
2016-Sep-04 23:09 UTC
[llvm-dev] Adding [[nodiscard]] to Compiler.h
On Sun, Sep 4, 2016 at 3:53 PM Sanjoy Das <sanjoy at playingwithpointers.com> wrote:> Hi Chandler, > > Chandler Carruth wrote: > > My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD. > > > > For compilers that support it, it should be a strict superset of > features and functionality. The standard feature was > > written directly based on the clang warn_unused_result stuff. > > Maybe I misunderstood something, but with clang 3.8.1 I cannot write: > > int [[clang::nodiscard]] f(); > > so I'm not sure if LLVM_NODISCARD provides a superset of the functionality. >I don't know anything about [[clang::nodiscard]], but I would expect [[clang::warn_unused_result]] in C++11 and C++14 to work in all the places and have the same effect as [[nodiscard]] does in C++17. Here is a recent clang build: echo -e '[[nodiscard]] int f();\nstruct [[nodiscard]] X {};' | clang -fsyntax-only -x c++ -std=c++1z - echo -e '[[clang::warn_unused_result]] int f();\nstruct [[clang::warn_unused_result]] X {};' | clang -fsyntax-only -x c++ -std=c++11 - -Chandler -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160904/9f77d00e/attachment-0001.html>
Justin Bogner via llvm-dev
2016-Sep-05 01:30 UTC
[llvm-dev] Adding [[nodiscard]] to Compiler.h
Sanjoy Das <sanjoy at playingwithpointers.com> writes:> Hi Chandler, > > Chandler Carruth wrote: >> My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD. >> >> For compilers that support it, it should be a strict superset of >> features and functionality. The standard feature was >> written directly based on the clang warn_unused_result stuff. > > Maybe I misunderstood something, but with clang 3.8.1 I cannot write: > > int [[clang::nodiscard]] f();That's the wrong spot - the attribute would apply to `int` in this expression. Also there's no [[clang::nodiscard]], it's either [[nodiscard]] or [[clang::warn_unused_result]]. Thus, you spell it like so: [[nodiscard]] int f();> so I'm not sure if LLVM_NODISCARD provides a superset of the functionality.As Chandler pointed out, [[nodiscard]] is almost exactly [[clang::warn_unused_result]], which is itself the same as clang's implementation of __attribute__((__warn_unused_result__)) modulo C++11 attributes being a bit more strict in terms of placement than gcc attributes.
Justin Bogner via llvm-dev
2016-Sep-05 01:40 UTC
[llvm-dev] Adding [[nodiscard]] to Compiler.h
Chandler Carruth <chandlerc at google.com> writes:> My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD. > > For compilers that support it, it should be a strict superset of features and > functionality. The standard feature was written directly based on the clang > warn_unused_result stuff. > > I would just migrate us onto the spelling and usage pattern that got > standardized. All we have to lose are warnings from compilers other than Clang > until they implement this feature. That seems like not a huge loss to me > honestly.You might be right. It seems a bit unfortunate that we'd lose these warnings on MSVC and gcc for the time being, but maybe that's better than the usability cost of having two macros that expand to the same thing. Do note though, if we do that this would mean clang is the only compiler we can support this on at all without -std=c++17, since [[nodiscard]] will trigger extension warnings in earlier standard modes.> -Chandler > > On Sun, Sep 4, 2016 at 2:57 PM Sanjoy Das via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > Hi Justin, > > Justin Bogner wrote: > > Sanjoy Das<sanjoy at playingwithpointers.com> writes: > >> Hi Justin, > >> > >> This SGTM generally, but please make the difference between > >> LLVM_NODISCARD and LLVM_UNUSED_RESULT clear in the code. :) > > > > Right, this is where it gets a little weird. LLVM_NODISCARD would be > for > > types, whereas LLVM_UNUSED_RESULT would be for functions. Depending on > > your host compiler, using the wrong one might DTRT, but it won't across > > all compilers. > > > > Do you think documenting this is sufficient, or should we try to name > > these to better represent where they should be used? > > Firstly, perhaps "LLVM_NODISCARD_TYPE" would be a better name? > > Secondly, if we define the following when the host compiler is clang, > > #define LLVM_UNUSED_RESULT __attribute__((warn_unused_result)) > #define LLVM_NODISCARD_TYPE [[clang::warn_unused_result]] > > via some shallow manual testing, it looks like using > LLVM_NODISCARD_TYPE on a function should break the build (though I'd > love it if someone more familiar with clang chimed in on this). So, is > the problem that we can accidentally use LLVM_UNUSED_RESULT on a type > and not know it? > > I think there is a dirty trick here -- we could: > > #define LLVM_UNUSED_RESULT __attribute__((warn_unused_result, enable_if > (true, ""))) > #define LLVM_NODISCARD_TYPE [[clang::warn_unused_result]] > > This breaks the (clang) build if LLVM_UNUSED_RESULT is used on a type. > > It would still be possible to not do the right thing on GCC or MSVC > and be able to build a binary, but I think as long as the clang bots > catch bad uses we're okay. > > -- Sanjoy` > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Chandler Carruth via llvm-dev
2016-Sep-05 02:16 UTC
[llvm-dev] Adding [[nodiscard]] to Compiler.h
On Sun, Sep 4, 2016 at 6:40 PM Justin Bogner <mail at justinbogner.com> wrote:> Chandler Carruth <chandlerc at google.com> writes: > > My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD. > > > > For compilers that support it, it should be a strict superset of > features and > > functionality. The standard feature was written directly based on the > clang > > warn_unused_result stuff. > > > > I would just migrate us onto the spelling and usage pattern that got > > standardized. All we have to lose are warnings from compilers other than > Clang > > until they implement this feature. That seems like not a huge loss to me > > honestly. > > You might be right. It seems a bit unfortunate that we'd lose these > warnings on MSVC and gcc for the time being, but maybe that's better > than the usability cost of having two macros that expand to the same > thing. > > Do note though, if we do that this would mean clang is the only compiler > we can support this on at all without -std=c++17, since [[nodiscard]] > will trigger extension warnings in earlier standard modes. >Yea, I dunno. If you end up wanting two macros, I'd suggest: LLVM_NODISCARD which works exactly as [[nodiscard]], and LLVM_NODISCARD_FUNCTION which works the way our current macro works. My two cents. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160905/164c94e3/attachment.html>
Justin Bogner via llvm-dev
2016-Oct-14 22:05 UTC
[llvm-dev] Adding [[nodiscard]] to Compiler.h
Justin Bogner <mail at justinbogner.com> writes:> Chandler Carruth <chandlerc at google.com> writes: >> My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD. >> >> For compilers that support it, it should be a strict superset of features and >> functionality. The standard feature was written directly based on the clang >> warn_unused_result stuff. >> >> I would just migrate us onto the spelling and usage pattern that got >> standardized. All we have to lose are warnings from compilers other than Clang >> until they implement this feature. That seems like not a huge loss to me >> honestly. > > You might be right. It seems a bit unfortunate that we'd lose these > warnings on MSVC and gcc for the time being, but maybe that's better > than the usability cost of having two macros that expand to the same > thing. > > Do note though, if we do that this would mean clang is the only compiler > we can support this on at all without -std=c++17, since [[nodiscard]] > will trigger extension warnings in earlier standard modes.After talking to a few people it sounds like one LLVM_NODISCARD macro is the way to go - We have prior art for clang-only warnings; we have enough coverage building with clang that we'll still catch anything quickly; and having two macros that are so similar is confusing and difficult to use. I'll stage this as adding an LLVM_NODISCARD, converting in tree code to use it instead of LLVM_ATTRIBUTE_UNUSED_RESULT, then finally removing the old macro.