Page MenuHomePhabricator

nlee (Namgoo Lee)
User

Projects

User does not belong to any projects.

User Details

User Since
May 10 2022, 10:09 AM (7 w, 1 d)

Recent Activity

Tue, Jun 14

nlee updated the diff for D125402: [clang][diag] warn if function returns class type by-const-value.

Warn if:

Tue, Jun 14, 8:21 PM · Restricted Project, Restricted Project

Sun, Jun 12

nlee added inline comments to D125402: [clang][diag] warn if function returns class type by-const-value.
Sun, Jun 12, 5:54 PM · Restricted Project, Restricted Project

Wed, Jun 8

nlee updated the diff for D125402: [clang][diag] warn if function returns class type by-const-value.

Updated diff to pass clang-format test.

Wed, Jun 8, 5:29 AM · Restricted Project, Restricted Project

Tue, Jun 7

nlee updated the diff for D125402: [clang][diag] warn if function returns class type by-const-value.

After some investigation, I think it is not always explicit that removing const makes any improvements when we are "constructing." For example, in this case, the two codes which differ in the return type of function f (S f() vs. const S f()) produce the same assembly output. I assume this is due to various copy elisions stepping in.

Tue, Jun 7, 9:42 AM · Restricted Project, Restricted Project

May 17 2022

nlee added a comment to D125402: [clang][diag] warn if function returns class type by-const-value.

Thank you for your reviews, and sorry for the mess in my last comment. I'm new to clang/Phabricator and trying to get used to it :)

May 17 2022, 7:19 AM · Restricted Project, Restricted Project

May 16 2022

nlee added a comment to D125402: [clang][diag] warn if function returns class type by-const-value.

Thank you for working on this new diagnostic! We don't typically add new diagnostics that are off by default unless they're for pedantic diagnostics or are reasonably expected to be enabled by default in the future. This is because we've had evidence that suggests such diagnostics aren't enabled often enough to warrant the maintenance cost for them.

I'm not convinced this diagnostic can be enabled by default. Yes, it prevents move semantics, but that's not typically an issue with the correctness of the code. IIRC, we decided to put this diagnostic into clang-tidy because we thought it would be too chatty in practice, especially on older code bases.

Perhaps there are ways we can improve the diagnostic behavior to allow it to be enabled by default though. One possibility would be to look at the return type to see if there's a user-provided move constructor (either = default or explicitly written) and only diagnose if one is found. But I think we'd want some evidence that this actually reduces the false positive rate in practice, which means trying to compile some large C++ projects (of various ages/code quality) to see. Would you be willing to run your patch against some large C++ projects to see what kind of true and false positives it generates? From there, we can decide whether we need additional heuristics or not.

May 16 2022, 8:21 PM · Restricted Project, Restricted Project

May 11 2022

nlee requested review of D125402: [clang][diag] warn if function returns class type by-const-value.
May 11 2022, 10:15 AM · Restricted Project, Restricted Project