Page MenuHomePhabricator

[analyzer][Dominators] Add unittests
ClosedPublic

Authored by Szelethus on May 29 2019, 10:30 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.May 29 2019, 10:30 AM
kuhar added inline comments.May 29 2019, 10:53 AM
clang/unittests/Analysis/CFGDominatorTree.cpp
95 ↗(On Diff #201977)

Why not have a constructor that takes the cfg and constructs a domtree straight away? But this should probably go into a separate patch.

113 ↗(On Diff #201977)

A tastcase with a virtual root postdominating other nodes would be welcome.

kuhar added inline comments.May 29 2019, 10:54 AM
clang/unittests/Analysis/CFGDominatorTree.cpp
37 ↗(On Diff #201977)

You can use a raw string literal to make it more readable: https://en.cppreference.com/w/cpp/language/string_literal

NoQ added inline comments.May 29 2019, 10:57 AM
clang/unittests/Analysis/CFGDominatorTree.cpp
37 ↗(On Diff #201977)

...or just omit \ns because the compiler doesn't really care.

Fixes according to reviewer comments, thanks! :)

Szelethus marked 5 inline comments as done.May 29 2019, 11:36 AM
Szelethus added inline comments.
clang/unittests/Analysis/CFGDominatorTree.cpp
95 ↗(On Diff #201977)

Yea, it's been bothering me for a while, but apparently not enough to fix it (just yet!).

Szelethus marked an inline comment as done.

Oh, I realized later that code I commented on were only moved from somewhere else. If you feel like tackling these comments feel free to do so in a separate patch so this one stays clean (no changes to moved code).

clang/unittests/Analysis/CFGBuilder.h
18 ↗(On Diff #202004)

Is it actually sensible to write tests where the tool failed? I can imagine these status codes being helpful for debugging but they has little to do with the tests. I wonder if we actually need all these or a nullable pointer as a result is sufficient.

54 ↗(On Diff #202004)

Do you expect the code to only contain one function? If so, you should enforce it.

65 ↗(On Diff #202004)

Do you need the std::move here?

Fixes according to reviewer comments, thanks! :)

Gentle ping :^)

NoQ added inline comments.Jun 25 2019, 1:27 PM
clang/unittests/Analysis/CFGBuilder.h
16 ↗(On Diff #202004)

Given that now it's in a header, i guess we need to pick a less generic name. Eg., CFGBuildResult.

54 ↗(On Diff #202004)

I guess it sucks that we can't re-use my findDeclByName() helper here.

clang/unittests/Analysis/CFGDominatorTree.cpp
22–28 ↗(On Diff #202004)

Why isn't this a simple function template?

30–32 ↗(On Diff #202004)

StmtType is unused. Did you mean FindStmt<StmtType>?

31 ↗(On Diff #202004)

This looks like "hasNoStmtType" to me, did you mean !=?

Szelethus marked an inline comment as done.Jun 25 2019, 4:31 PM
Szelethus added inline comments.
clang/unittests/Analysis/CFGDominatorTree.cpp
22–32 ↗(On Diff #202004)

Hmmm, this entire thing is pointless, really. It is used as a sanity check of some sort, whether for example the 4th block really is what I believe it would be. But this is anything but a reliable way to test it.

I think this causes far more confusion than value, I'll just remove it, the actual tests are thorough enough enough to break if the CFG changes.

Szelethus updated this revision to Diff 207759.Jul 3 2019, 5:06 AM

Fixes according to reviewer comments!

Szelethus marked 3 inline comments as done.Jul 3 2019, 5:07 AM
xazax.hun accepted this revision.Jul 3 2019, 11:36 AM

Looks good to me, some nits inline.

clang/unittests/Analysis/CFGBuildResult.h
1 ↗(On Diff #207759)

Filename not updated?

clang/unittests/Analysis/CFGDominatorTree.cpp
1 ↗(On Diff #207759)

Filename not updated?

13 ↗(On Diff #207759)

Do you need the Tooling header here?

This revision is now accepted and ready to land.Jul 3 2019, 11:36 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2019, 3:17 AM