C++ attributes and virtual functions

15 views
Skip to first unread message

Daniel Cheng

unread,
Jan 20, 2022, 9:12:16 PMJan 20
to cxx, hi...@chromium.org
There are a few places in our codebase that use [[nodiscard]] for a virtual function: https://source.chromium.org/search?q=%22%5B%5Bnodiscard%5D%5D%20virtual%22&sq=&ss=chromium

However, C++ attributes are not inherited: https://godbolt.org/z/TKrYPGT58

The C++ style guide disallows default arguments on virtual functions due to the potentially surprising interaction between the two features: https://google.github.io/styleguide/cppguide.html#Default_Arguments

Do we want to disallow C++ attributes for virtual methods? Or do we want to have our toolchain enforce consistency of the attributes for overridden methods? Something else altogether?

Daniel

Jeremy Roman

unread,
Jan 20, 2022, 9:26:29 PMJan 20
to Daniel Cheng, cxx, hi...@chromium.org
Disallowing seems like we lose the opportunity to get useful warnings at least sometimes, so at least for [[nodiscard]] that seems bad.

It might be a useful warning if [[nodiscard]] is present on the base but not on the overridden member, though such a warning might even make sense in upstream Clang. Do you know if that's been investigated?

--
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/CAF3XrKraLQQQezED%3DJsb4_c3Dn0gD7R-dMg_YBNmC2hwMjcYyg%40mail.gmail.com.

Roland McGrath

unread,
Jan 20, 2022, 10:02:08 PMJan 20
to Jeremy Roman, Daniel Cheng, cxx, hi...@chromium.org
It does not seem like a real parallel to the default arguments case.  There the potentially surprising behavior is getting different defaults depending on which scope made the call (i.e. whether it was via the base class or via the derived class).  In the case of [[nodiscard]], the only surprise is missing a warning that you wanted to get, not getting different behavior.  So I think just having [[nodiscard]] on base classes (i.e. where the API is specified) and nothing more is probably getting 80%+ of the intended benefit of [[nodiscard]].  I do think that a Clang warning for mismatched caller-relevant attributes like [[nodiscard]] would be a good improvement.  But I think even without that, disallowing [[nodiscard]] for any class of cases would be only a negative for code health.

Mark Mentovai

unread,
Jan 20, 2022, 10:23:36 PMJan 20
to Jeremy Roman, Daniel Cheng, cxx, hi...@chromium.org
Don’t disallow.

A warning for not re-declaring [[nodiscard]] on an override might be warranted most of the time, but not always. A not-too-contrived case:

class FileWriter {
 public:
  [[nodiscard]] virtual bool Write(void const *, size_t) = 0;
};

// Wraps a std::string.
class StringFile final : public FileWriter {
 public:
  // Practically, this can’t fail.
  bool Write(void const *, size_t) override;
};

If you know you’re holding a StringFile, there’s no reason to check Write’s return value, but if you’re accessing through a FileInterface and writes might fail, [[nodiscard]] is right. The full benefit of [[nodiscard]] can be realized if all you have is a FileInterface and no additional special knowledge of the implementation.

Having a warning for this pattern would mean that we’d need [[nonodiscard]] to overcome it when appropriate (more realistically, a #pragma clang diagnostic would cover it).

Also, attributes cover a pretty broad spectrum of applications, I don’t think it’s valuable to consider disallowing all attributes in a particular context (as this thread does for virtual functions) although there may certainly be reasons to consider specific attributes.

Mark

Roland McGrath

unread,
Jan 20, 2022, 10:28:49 PMJan 20
to Mark Mentovai, Jeremy Roman, Daniel Cheng, cxx, hi...@chromium.org
I think your example is a good case for having the warning ensuring consistent [[nodiscard]] on the derived class and then just using `std::ignore = string_file.Write(...);` in the places where you know it's fine.  When reading the code, the cognitive load of thinking about "this version of the FIleWriter API contract" vs "the FileWriter API contract" IMHO overwhelms the nuisance of writing and reading `std::ignore = ` on each use where you have good reason to apply a different contract than the API normally has.

Daniel Cheng

unread,
Jan 21, 2022, 1:22:58 AMJan 21
to Roland McGrath, Mark Mentovai, Jeremy Roman, cxx, hi...@chromium.org
> It does not seem like a real parallel to the default arguments case.

The parallel is that the behaviour (in this case, compile-time behaviour) differs based on the static type for the object pointer.

For what it's worth, my personal feeling is we should encourage attributes on overrides to match, but I wanted to make sure several options were presented. I do not think that addressing this should block any other work, but I did think it was worth raising the issue.

Does anyone have experience working with the Clang team to figure out if a warning would make sense here?

Daniel

dan...@chromium.org

unread,
Jan 21, 2022, 12:08:47 PMJan 21
to Daniel Cheng, Nico Weber, Hans Wennborg, Roland McGrath, Mark Mentovai, Jeremy Roman, cxx, hi...@chromium.org
On Fri, Jan 21, 2022 at 1:23 AM Daniel Cheng <dch...@chromium.org> wrote:
> It does not seem like a real parallel to the default arguments case.

The parallel is that the behaviour (in this case, compile-time behaviour) differs based on the static type for the object pointer.

For what it's worth, my personal feeling is we should encourage attributes on overrides to match, but I wanted to make sure several options were presented. I do not think that addressing this should block any other work, but I did think it was worth raising the issue.

Does anyone have experience working with the Clang team to figure out if a warning would make sense here?

Reply all
Reply to author
Forward
0 new messages