C++17 feature proposal: Allow [[nodiscard]]

[Daniel Cheng]

This is the C++17 version of WARN_UNUSED_RESULT. It can also be used on structs/classes/enums to indicate that the type must not be ignored when returned by a function: https://godbolt.org/z/3TEP959KE

As part of allowing this, we would remove WARN_UNUSED_RESULT and convert all existing uses to [[nodiscard]].

Incidentally, do we have a preferred style when something is annotated with multiple attributes?

Daniel

[Avi Drissman]

+1 to allowing it, and to the migration and removal of the macro.

--
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/CAF3XrKpjUyXBBYB_QLhJyymCss8bgAnCp%2BGYn_145w7AEofZKg%40mail.gmail.com.

[Peter Kasting]

On Thu, Jan 6, 2022 at 12:26 PM Daniel Cheng <dch...@chromium.org> wrote:

As part of allowing this, we would remove WARN_UNUSED_RESULT and convert all existing uses to [[nodiscard]].

+1

Also bonus points if you add the clang-tidy "modernize use nodiscard" pass.

Incidentally, do we have a preferred style when something is annotated with multiple attributes?

This is legal, right?  [[fallthrough, nodiscard]]  (Ignore the fact that those two make no sense together)

If so, I'd do that, with the annotations in alphabetical order.

PK

[Jeremy Roman]

On Thu, Jan 6, 2022 at 3:59 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Thu, Jan 6, 2022 at 12:26 PM Daniel Cheng <dch...@chromium.org> wrote:

As part of allowing this, we would remove WARN_UNUSED_RESULT and convert all existing uses to [[nodiscard]].

+1

Also bonus points if you add the clang-tidy "modernize use nodiscard" pass.

Incidentally, do we have a preferred style when something is annotated with multiple attributes?

This is legal, right?  [[fallthrough, nodiscard]]  (Ignore the fact that those two make no sense together)

Yes, that is legal syntax.

 

If so, I'd do that, with the annotations in alphabetical order.

Honestly I suspect we don't need to specify this. I would expect it to be rather uncommon and when it does occur the author and reviewer are likely to make reasonable choices (the same as how we don't, e.g., specify the order in which local variables are declared). If upstream C++ style hasn't felt the need to ask for this, then we probably don't need to either.

 

PK

--
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/CAAHOzFACJbze%3DUubB5FMmRuDLd252yi8XEVGkoFM4Nic5aqtqg%40mail.gmail.com.

[Jeremy Roman]

also +1

[Daniel Cheng]

We've migrated most instances of WARN_UNUSED_RESULT to [[nodiscard]]. However, there's an interesting question of what we should do for Objective-C/Objective-C++ code.

Somewhat surprisingly, it seems that C++ attributes work:

$ third_party/llvm-build/Release+Asserts/bin/clang -ObjC++ test.mm
test.mm:13:3: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
  [self test];
  ^

Though not in regular Objective-C:

$ third_party/llvm-build/Release+Asserts/bin/clang -ObjC test.mm
test.mm:2:12: error: expected ';' after method prototype
- (int)test [[nodiscard]];
           ^
           ;

It seems like we have a few Objective-C files in the tree:

$ git ls-files | grep "\.m$" | grep -v third_party
components/cronet/ios/cronet_consumer/cronet_consumer_view_controller.m
ios/chrome/browser/ui/material_components/chrome_app_bar_view_controller.m
ios/web_view/shell/shell_app_delegate.m
ios/web_view/shell/shell_auth_service_fake.m
ios/web_view/shell/shell_autofill_delegate.m
ios/web_view/shell/shell_exe_main.m
ios/web_view/shell/shell_risk_data_loader_fake.m
ios/web_view/shell/shell_translation_delegate.m
ios/web_view/shell/shell_trusted_vault_provider_fake.m
ios/web_view/shell/shell_view_controller.m
remoting/ios/mdc/MDCActionImageView.m
tools/mac/icons/makeicns2.m

And [[nodiscard]] definitely will not work for those. Do the Objective-C experts have any opinions on how we should approach this? I can think of several approaches:

1. Add base/mac/warn_unused_result.h for use in Objective-C/Objective-C++ code and ban [[nodiscard]] for Objective-C interfaces.

2. Move all the .m files to .mm files and use [[nodiscard]] everywhere, even in Objective-C interfaces.

3. Kick the can down the road. Use [[nodiscard]] everywhere since no Objective-C files currently use WARN_UNUSED_RESULT and hope for the best.

I'm assuming we'll end up with some variation of #1 or #2, but would love to hear feedback.

Daniel

[Sylvain Defresne]

I've looked at all the iOS files and I think they can all be converted to Objective-C++ except the files below //ios/web_view/shell.
The files in //ios/web_view/shell are part of a sample application built on top of the ChromeWebView framework (a shared library with corresponding public headers). This framework exposes an Objective-C API. The framework is used by third-party projects whose code is Objective-C.

Compiling those files as pure Objective-C allows us to confirm that the exposed API does not use C++ features (which would break the third-party projects). Those files however do not depend on anything in Chromium, they just use the WebView API, so they should not prevent using [[nodiscard]] in Chrome.

The reason is that, apart from the public headers exposed by WebView, all the other files could end up using other parts of Chromium implemented in C++ and should be converted to Objective-C++ at that time.

So my recommendation would be to convert all iOS files to Objective-C++ (except those in //ios/web_view/shell), allow [[nodiscard]] everywhere (except in public headers that forms WebView, but they can't use WARN_UNUSED_RESULT today), and disallow Objective-C files outside of //ios/web_view/shell (and possibly other similar test applications build to check a framework distributed to third-party).

For the lone macOS file, it looks like it could be converted to Objective-C++, but I may not be the correct person to make the call.

I guess, this is a mix of #2 and #3.

-- Sylvain

[Mark Mentovai]

This is expected. Those are Objective-C files and were never able to mix with C++, and that’s not a problem.

tools/mac/icons/makeicns2.m doesn’t have any Chrome dependencies and, in fact, isn’t even part of any build. It is our own code, but an infrequently-used tool that we use to pack icons into the format Apple uses. Since it doesn’t use any other Chrome code, it won’t see any Chrome code’s use of [[nodiscard]]. #3 is the correct option here.

Stepping outside of mac and into ios, aside from ios/web_view for which Sylvain gave a most definitive answer, none of the remaining files actually have any sort of dependency on any other Chrome code either. I think that these can all safely be left at #3.

This isn’t any sort of problem at all, really, when you think about it. These Objective-C-but-not-C++ files were never able to see any C++, even through an #include, or their compilation would have broken. It’s true that if they saw [[nodiscard]] now, their compilation would break, but that’s because [[nodiscard]] is C++, and these files aren’t expecting to see C++. If they did need to access C++ because of a Chromium #include or any other reason, it could be addressed at this time. [[nodiscard]] in unrelated code is not a forcing function to artificially make these files compile as C++. This favors #3 across the board, without any fear of repercussion.

On that basis, I’m completely comfortable calling this a genuine “mission accomplished” and sharing due congratulations.

Mark