This is an archive of the discontinued LLVM Phabricator instance.

[CTU] Add more lit tests and better error handling
ClosedPublic

Authored by martong on Nov 30 2018, 9:25 AM.

Details

Summary

Adding some more CTU list tests. E.g. to check if a construct is unsupported.
We also slightly modify the handling of the return value of the Import
function from ASTImporter.

Diff Detail

Event Timeline

martong created this revision.Nov 30 2018, 9:25 AM
Szelethus added inline comments.Nov 30 2018, 9:37 AM
test/Analysis/ctu-main.c
4–6

Could you please break these lines?

// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu %S/Inputs/ctu-other.c \
// RUN:    -emit-pch -o %t/ctudir2/ctu-other.c.ast
//
// RUN: cp %S/Inputs/externalFnMap2.txt %t/ctudir2/externalFnMap.txt
//
// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c89 -analyze -verify %s
// RUN:   -analyzer-checker=core \
// RUN:   -analyzer-checker=debug.ExprInspection \
// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
// RUN:   -analyzer-config ctu-dir=%t/ctudir2
5

This is a question rather than anything else, why do we have both externalFnMap2.txt and externalFnMap.txt?

Also, maybe it'd be worth making a CTU directory under test/Analysis for CTU related test files.

Hi Gabor,
Please find my comments inline.

lib/CrossTU/CrossTranslationUnit.cpp
251

Conditionals with a single-line body don't require braces.

test/Analysis/Inputs/ctu-other.c
2

Please clang-format the new files.

7

Please use a consistent naming style across the file. There are names starting with capital, having underscores and written like this.

martong updated this revision to Diff 176619.Dec 4 2018, 6:30 AM
martong marked 8 inline comments as done.
  • Break long RUN lines
  • Clang format the test files
  • Use consistent naming style
  • Remove braces
test/Analysis/Inputs/ctu-other.c
7

Ok, I renamed the things, I was trying to be consistent with the LLVM style, one exception is the name of variables still start with small case.
I also restructured the file a bit and added more comments, so it is more cleaner which things belong together to the same test.

test/Analysis/ctu-main.c
4–6

Ok, good point.

5

externalFnMap.txt goes for ctu-other.cpp.
externalFnMap2.txt goes for ctu-other.c.
Perhaps we should rename them in the Inputs directory to include the indexed file name. E.g. ctu-other.cpp.externalFnMap.txt. What do you think?

Also, maybe it'd be worth making a CTU directory under test/Analysis for CTU related test files.

It is a good point, but I'd do that in the future once we have even more ctu related test files. Perhaps together with a new check-clang-analysis-ctu build target.

martong marked an inline comment as done.Dec 5 2018, 8:44 AM
martong added inline comments.
lib/CrossTU/CrossTranslationUnit.cpp
247

TODO: add a comment that this function never returns with nullptr on success. And consequently CrossTranslationUnitContext::getCrossTUDefinition neither.

Szelethus added inline comments.Dec 5 2018, 9:21 AM
test/Analysis/ctu-main.c
5

Sounds great! :)

martong marked 2 inline comments as done.Dec 7 2018, 3:44 AM
martong added inline comments.
test/Analysis/ctu-main.c
5

Ok, I have renamed the ExternalFnMap files.

martong updated this revision to Diff 177170.Dec 7 2018, 3:45 AM
martong marked an inline comment as done.
  • Rename externalFnMap files
This revision is now accepted and ready to land.Dec 7 2018, 7:38 AM
This revision was automatically updated to reflect the committed changes.
test/Analysis/Inputs/ctu-other.c