This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Prototype to check for exception specification
AbandonedPublic

Authored by JonasToth on Mar 25 2017, 9:10 AM.

Details

Summary

This check is a first prototype to analyze the exception specification of source code.
Its goal is to annotate function correctly and automatically whenever possible and
find places where noexcept-rules are violated.
It is meant to be a basis to check if destructors and deallocation functions are truly
noexcept.

The current state has some Problems and i added notes on code places i dont like, but dont
know how to do better. Its not meant to be merged directly, but more as a discussion
basis.

Event Timeline

JonasToth created this revision.Mar 25 2017, 9:10 AM
JonasToth added inline comments.Mar 25 2017, 9:13 AM
clang-tidy/modernize/NoexceptCorrectnessCheck.cpp
28

I could not figure out how to refactor hasDirectThrow, hasIndirectThrow and hasThrowingExpression properly.
Creating functions that would return these matchers, resulted in compile errors while binding. I could not see the problem in all the template errors. Maybe someone else can tell me how to do this correctly.

60

How can i find out the exception spepcification in a FunctionDecl? I would like to check if noexcept might be added when it is not justified.

JonasToth updated this revision to Diff 93045.Mar 25 2017, 9:15 AM
  • [Doc] short descript in header added
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added subscribers: Prazek, cfe-commits.

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

docs/clang-tidy/checks/modernize-noexcept-correctness.rst
8

Please highlight language constructs with ``.

17

Please run Clang-format over all code snippets.

42

Please highlight language constructs with ``.

jbcoe added a subscriber: jbcoe.Mar 25 2017, 12:08 PM
mgehre added a subscriber: mgehre.Mar 25 2017, 2:45 PM
mgehre added inline comments.
clang-tidy/modernize/NoexceptCorrectnessCheck.cpp
49

dynamic_cast can only throw if the new type is a reference type.

60

Try ThrowingDecl ->getType()->getAs<FunctionProtoType>()->getNoexceptSpec()

test/clang-tidy/modernize-noexcept-correctness.cpp
5

I find it helpful to get fixits for adding "noexcept", but not so much for adding noexcept(false) because this is already the default. I would prefer to hide generation of fixits that state the default behind an option.

43

Use FIXME here?

49

Use FIXME here?

JonasToth planned changes to this revision.Mar 28 2017, 1:41 AM
JonasToth marked 2 inline comments as done.

commented review

clang-tidy/modernize/NoexceptCorrectnessCheck.cpp
60

Does not build :/

note: candidate: clang::FunctionProtoType::NoexceptResult clang::FunctionProtoType::getNoexceptSpec(const clang::ASTContext&) const
NoexceptResult getNoexceptSpec(const ASTContext &Ctx) const;

Where do i get the ASTContext from?

docs/clang-tidy/checks/modernize-noexcept-correctness.rst
42

should i highlight function here and above?
as well exception. I think they are not directly language constructs but more concepts.

test/clang-tidy/modernize-noexcept-correctness.cpp
5

yes. i plan to introduce an option for that later (its in the doc mentioned). but first I try to figure out a solid way to find direct, indirect and nonthrowing functions :)

43

this line shall cause a warning. CHECK-MESSAGE inserted.

JonasToth marked an inline comment as done.Mar 28 2017, 1:57 AM
JonasToth added inline comments.
clang-tidy/modernize/NoexceptCorrectnessCheck.cpp
60

FunctionPrototype has the isNothrow method, but it needs the ASTContext as well.
An alternative solution would be hasNoexceptExceptionSpec(). It would ignore throw() most likely.

aaron.ballman added inline comments.Mar 28 2017, 5:03 AM
clang-tidy/modernize/NoexceptCorrectnessCheck.cpp
60

Result.Context is the ASTContext you need.

Eugene.Zelenko added inline comments.Mar 28 2017, 7:02 AM
docs/clang-tidy/checks/modernize-noexcept-correctness.rst
42

No, only noexcept(false).

mboehme added inline comments.Mar 30 2017, 3:04 AM
docs/clang-tidy/checks/modernize-noexcept-correctness.rst
8

Omit comma before "that".

28

Typo: "Definitely"

test/clang-tidy/modernize-noexcept-correctness.cpp
62

Typos: "too complex", "left alone"

this check is somewhat duplicated to a check currently in review, which seemed to be abondened?

https://reviews.llvm.org/D19201

I will read that one first and try to coordinate with the author.

The matcher in D33537 could be reused here as well. Maybe I should make it common, but it is more complex than any of the common matchers.

The matcher in D33537 could be reused here as well. Maybe I should make it common, but it is more complex than any of the common matchers.

yes, the matcher seems interesting. but i think i will abondon this check and integrate the functionality somewhere else (maybe ur check), since it seems to be too much duplication otherwise.
for now, i dont have a lot of time, so it is stall. hopefully, this will change in the next month or so.

JonasToth abandoned this revision.Aug 24 2017, 9:18 AM

exception checking seems to end in the front end for such cases, so nothing to do here anymore. reimplement if there is need later.