Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,8 @@ Improvements to Clang's diagnostics
``ACQUIRED_AFTER(...)`` have been moved to the stable feature set and no
longer require ``-Wthread-safety-beta`` to be used.

- The `-Wnrvo` compiler flag is now ignored in C mode.

Improvements to Clang's time-trace
----------------------------------

Expand Down
6 changes: 4 additions & 2 deletions clang/lib/Sema/SemaDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16174,8 +16174,10 @@ void Sema::computeNRVO(Stmt *Body, FunctionScopeInfo *Scope) {
for (unsigned I = 0, E = Scope->Returns.size(); I != E; ++I) {
if (const VarDecl *NRVOCandidate = Returns[I]->getNRVOCandidate()) {
if (!NRVOCandidate->isNRVOVariable()) {
Diag(Returns[I]->getRetValue()->getExprLoc(),
diag::warn_not_eliding_copy_on_return);
if (getLangOpts().CPlusPlus) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a nested if, just merge the two conditions using &&.

Copy link
Author

@javiermunozkirschberg javiermunozkirschberg Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

One question, after the Diag, and as part of if(!NRVOCandidate) there is a
Returns[I]->setNRVOCandidate(nullptr);

If I merge the two ifs(), then diag will be skip, but returns[i]->setNRVOCandidate(nullptr) will be also skipped. Is that ok?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no, disregard that. I misread that because of how the diff was formatted; we do in fact need to keep them separate yes.

Diag(Returns[I]->getRetValue()->getExprLoc(),
diag::warn_not_eliding_copy_on_return);
}
Returns[I]->setNRVOCandidate(nullptr);
}
}
Expand Down
41 changes: 41 additions & 0 deletions clang/test/SemaCXX/no-warn-nrvo-on-c.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// RUN: %clang -std=c23 -Wnrvo -Xclang -verify %s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// RUN: %clang -std=c23 -Wnrvo -Xclang -verify %s
// RUN: %clang_cc1 -std=c23 -Wnrvo -verify %s

// expected-no-diagnostics

#include <stdlib.h>

#define SIZE 20

typedef struct String_s {
char* buf;
size_t len;
} String;


void clean(String* s) {
free(s->buf);
}

String randomString() {
String s = {};

s.buf = malloc(SIZE);
s.len = SIZE;

if (!s.buf) {
goto fail;
}

return s;

fail:
clean(&s);
return (String){};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also try returning from the body of an if statement as well.

}

int main(int argc, char** argv)
{
String s= randomString();
clean(&s);

return 0;
}
Comment on lines +35 to +41
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: lets call that functions something else void test() would do - clang test files are never executed and having main functions is confusing

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this, thanks!