Page MenuHomePhabricator

[CTU] Add statistics
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Nov 30 2018, 9:37 AM

Hi Gabor,
Sorry, but I don't understand the meaning of some options. Could you please explain what are NumNoUnit and NumNotInOtherTU and what is the difference between them?

xazax.hun accepted this revision.Dec 4 2018, 6:59 AM

The code LGTM! I am not good at wordsmithing, but if the descriptions of the statistics are not clear enough, I agree that they should be rephrased.

This revision is now accepted and ready to land.Dec 4 2018, 6:59 AM
martong updated this revision to Diff 176654.Dec 4 2018, 9:14 AM
  • Remove NumNoUnit

Sorry, but I don't understand the meaning of some options. Could you please explain what are NumNoUnit and NumNotInOtherTU and what is the difference between them?

Your point is absolutely true. They are the same, I think at some point some time ago they were different (in our fork), now it is just redundancy. So I removed NumNoUnit.

martong updated this revision to Diff 176655.Dec 4 2018, 9:22 AM
martong marked an inline comment as done.
  • Remove braces

> Sorry, but I don't understand the meaning of some options. Could you please explain what are NumNoUnit and NumNotInOtherTU and what is the difference between them?

Your point is absolutely true. They are the same, I think at some point some time ago they were different (in our fork), now it is just redundancy. So I removed NumNoUnit.

They are the same because in loadExternalAST we must return witn a non nullptr value, so it seems like the second check is redundant here:

if (!ASTUnitOrError) {
  return ASTUnitOrError.takeError();
}
ASTUnit *Unit = *ASTUnitOrError;
if (!Unit)
  return llvm::make_error<IndexError>(
      index_error_code::failed_to_get_external_ast);

I am going to open another patch to fix this.

a_sidorin accepted this revision.Dec 5 2018, 1:53 PM

LGTM with a nit.

lib/CrossTU/CrossTranslationUnit.cpp
171 ↗(On Diff #176655)

We can omit braces here.

Closed by commit rL348584: [CTU] Add statistics (authored by martong, committed by ). · Explain WhyDec 7 2018, 3:58 AM
This revision was automatically updated to reflect the committed changes.
martong marked 2 inline comments as done.Dec 7 2018, 4:03 AM
martong added inline comments.
lib/CrossTU/CrossTranslationUnit.cpp
171 ↗(On Diff #176655)

Ok, thanks, I changed that.