This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] check for __func__/__FUNCTION__ in lambdas
ClosedPublic

Authored by brycel on May 24 2017, 7:41 AM.

Details

Summary

Add a clang-tidy check for using func__/FUNCTION__ inside lambdas. This evaluates to the string operator(), which is almost never useful and almost certainly not what the author intended.

Diff Detail

Repository
rL LLVM

Event Timeline

brycel created this revision.May 24 2017, 7:41 AM
  1. What about __PRETTY_FUNCTION__ ?
  2. Consider following generic error handling macro:

(ThrowException is some template function)

#undef STR
#define STR(a) XSTR(a)

#define ThrowExceptionHelper(CLASS, fmt, ...) ThrowException<CLASS>(__FILE__ ":" STR(__LINE__) ": %s: " fmt, __PRETTY_FUNCTION__, ##__VA_ARGS__)
#endif

Which is called like if(somethig) ThrowException(std::exception, "%s", "bar");
Even though the function name may be useless, file/line info is still there and is(?) correct.
Perhaps there should not be a warning in such a case?

  1. What about __PRETTY_FUNCTION__ ?

__PRETTY_FUNCTION__ is a little more useful, it at least tells you which function the lambda is defined in:

#include <stdio.h>

int main() {
  auto f = [] {
    printf("__func__: %s\n", __func__);
    printf("__FUNCTION__: %s\n", __FUNCTION__);
    printf("__PRETTY_FUNCTION__: %s\n", __PRETTY_FUNCTION__);
  };
  f();
}

results in the output:

__func__: operator()
__FUNCTION__: operator()
__PRETTY_FUNCTION__: auto main()::(anonymous class)::operator()() const

I think __PRETTY_FUNCTION__ inside a lambda is useful enough not to warn about.

  1. Consider following generic error handling macro:

(ThrowException is some template function)

#undef STR
#define STR(a) XSTR(a)

#define ThrowExceptionHelper(CLASS, fmt, ...) ThrowException<CLASS>(__FILE__ ":" STR(__LINE__) ": %s: " fmt, __PRETTY_FUNCTION__, ##__VA_ARGS__)
#endif

Which is called like if(somethig) ThrowException(std::exception, "%s", "bar");
Even though the function name may be useless, file/line info is still there and is(?) correct.
Perhaps there should not be a warning in such a case?

That's a good point. I'll look into suppressing the warning if we're in a macro definition where __FILE__ and __LINE__ are both used. It may suppress some warnings that would be legitimate (such as if __FILE__/__LINE__ are written but not actually output anywhere), but in those cases it's probably impossible to figure out the author's intention.

  1. What about __PRETTY_FUNCTION__ ?

I think __PRETTY_FUNCTION__ inside a lambda is useful enough not to warn about.

Then that should be checked by the test

  1. Consider following generic error handling macro:

(ThrowException is some template function)

That's a good point. I'll look into suppressing the warning if we're in a macro definition where __FILE__ and __LINE__ are both used. It may suppress some warnings that would be legitimate (such as if __FILE__/__LINE__ are written but not actually output anywhere), but in those cases it's probably impossible to figure out the author's intention.

Makes sense

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Eugene.Zelenko retitled this revision from clang-tidy check for __func__/__FUNCTION__ in lambdas to [clang-tidy] check for __func__/__FUNCTION__ in lambdas.May 24 2017, 10:15 AM
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a project: Restricted Project.
brycel updated this revision to Diff 100149.May 24 2017, 12:55 PM

Addressed the following review comments:

  • Added a test to make sure we don't warn on PRETTY_FUNCTION
  • Suppressed warnings when in a macro that also uses FILE and LINE
  • Updated docs/ReleaseNotes.rst.
Eugene.Zelenko added inline comments.May 24 2017, 1:19 PM
docs/ReleaseNotes.rst
83 ↗(On Diff #100149)

Please highlight func and FUNCTION with ``.

brycel updated this revision to Diff 100159.May 24 2017, 1:47 PM
brycel marked an inline comment as done.

Fixed ReleaseNotes.rst

Eugene.Zelenko added inline comments.May 24 2017, 2:19 PM
docs/ReleaseNotes.rst
83 ↗(On Diff #100149)

I meant double `, not single .

brycel updated this revision to Diff 100161.May 24 2017, 2:22 PM

Double backticks, not single.

brycel marked an inline comment as done.May 24 2017, 2:22 PM
brycel added inline comments.
docs/ReleaseNotes.rst
83 ↗(On Diff #100149)

Sorry, I had Markdown on my brain.

alexfh added inline comments.May 30 2017, 5:05 AM
clang-tidy/misc/LambdaFunctionNameCheck.cpp
61–62 ↗(On Diff #100161)

llvm::make_unique<MacroExpansionsWithFileAndLine>(...)

67–68 ↗(On Diff #100161)

nit: I'd use the early return style here.

73–74 ↗(On Diff #100161)

I'm afraid this can become extremely slow on large files with boilerplate/generated code. Since the code is just looking for exact matches (and not any overlapping ranges, for example), we could replace the vector with a (hash?) map to limit the worst case complexity.

brycel updated this revision to Diff 101223.Jun 2 2017, 8:56 AM
brycel marked 4 inline comments as done.

Addressed comments from alexfh. In particular, changed to using a set from a vector.

brycel updated this revision to Diff 101224.Jun 2 2017, 9:04 AM

Move namespace-level types to class-level to avoid potential future name conflicts.

alexfh accepted this revision.Jun 2 2017, 10:05 AM

Looks good!

This revision is now accepted and ready to land.Jun 2 2017, 10:05 AM
This revision was automatically updated to reflect the committed changes.