Exactly what it says on the tin!
Details
Diff Detail
Event Timeline
clang/unittests/Analysis/CFGDominatorTree.cpp | ||
---|---|---|
38 | You can use a raw string literal to make it more readable: https://en.cppreference.com/w/cpp/language/string_literal |
clang/unittests/Analysis/CFGDominatorTree.cpp | ||
---|---|---|
38 | ...or just omit \ns because the compiler doesn't really care. |
clang/unittests/Analysis/CFGDominatorTree.cpp | ||
---|---|---|
96 | Yea, it's been bothering me for a while, but apparently not enough to fix it (just yet!). |
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 | 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 | Do you expect the code to only contain one function? If so, you should enforce it. | |
65 | Do you need the std::move here? |
clang/unittests/Analysis/CFGBuilder.h | ||
---|---|---|
16 | Given that now it's in a header, i guess we need to pick a less generic name. Eg., CFGBuildResult. | |
54 | I guess it sucks that we can't re-use my findDeclByName() helper here. | |
clang/unittests/Analysis/CFGDominatorTree.cpp | ||
22–28 | Why isn't this a simple function template? | |
30–32 | StmtType is unused. Did you mean FindStmt<StmtType>? | |
31 | This looks like "hasNoStmtType" to me, did you mean !=? |
clang/unittests/Analysis/CFGDominatorTree.cpp | ||
---|---|---|
22–32 | 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. |
Given that now it's in a header, i guess we need to pick a less generic name. Eg., CFGBuildResult.