This is an archive of the discontinued LLVM Phabricator instance.

Record macro references in #ifdef clause.
ClosedPublic

Authored by VitaNuo on Nov 23 2022, 3:27 AM.

Details

Summary

Records macro references in #ifdef clauses as ambiguous.

Diff Detail

Event Timeline

VitaNuo created this revision.Nov 23 2022, 3:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2022, 3:27 AM
VitaNuo requested review of this revision.Nov 23 2022, 3:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2022, 3:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks, this code looks good to me overall. I think we can extend it to handle the Defined, Elifdef, Elifndef cases.

And please add a [include-cleaner] prefix in the commit title.

clang-tools-extra/include-cleaner/lib/Record.cpp
103

nit: we can set a default value (RefType::Explicit) for the RT parameter, then we don't need to pass the RefType::Explicit in all callsites.

VitaNuo updated this revision to Diff 477551.Nov 23 2022, 10:10 AM

Remove the extra test.

VitaNuo updated this revision to Diff 477554.Nov 23 2022, 10:15 AM

Make explicit RefType default.

hokein added inline comments.Nov 24 2022, 1:45 AM
clang-tools-extra/include-cleaner/lib/Record.cpp
103

this line exceeds the max 80 character limit, clang-format should fix it.

103

the indentation doesn't look right, running clang-format should fix it.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
228

I think we can simplify the testcase further -- the header.h is not needed, we can use the macro X in all tests (#ifdef, #ifndef etc)

233

the #define Y 2 can be removed, our test doesn't care about it. just use

#ifdef X
#endif

is enough.

248

nit: this can be removed, as the EXPECT_THAT on line 258 covers this.

255

we're using the index of the array Recorded.MacroRefrences to access another array, it is fine currently (because both arrays has the same size), but we will get an out-of-bound-access issue if we add a case #if defined(X)..to the MainFile.

If we just use a single macro X for all testcases, then the RefdNames is not needed, we check EXPECT_EQ("X", Ref.Target.macro().Name->getName());.

VitaNuo updated this revision to Diff 477707.Nov 24 2022, 2:00 AM

Add support for #if defined(X), #elifdef, #elifndef

VitaNuo updated this revision to Diff 477711.Nov 24 2022, 2:26 AM

Address review comments. Format and simplify code.

clang-tools-extra/include-cleaner/lib/Record.cpp
103

Right, didn't know default parameter values were possible in C++, thanks.

103

Formatting should be fine now.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
228

Ok. I was trying to add some variety, but you're right - it's not the job of this test to check that things are included properly.

233

Ok, I'll remove all the fluff.

255

Ok, I've reduced everything to X.

VitaNuo marked 5 inline comments as done.Nov 24 2022, 2:29 AM
VitaNuo added inline comments.
clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
248

Sorry, not sure what this comment refers to. Can it be that the line numbering changed due to my newer patch, and this comment does not show up in the correct place anymore?

VitaNuo updated this revision to Diff 477716.Nov 24 2022, 2:39 AM

Formatting.

hokein added inline comments.Nov 24 2022, 3:13 AM
clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
235

nit: we can get rid of the main function, it is not needed.

238

nit: this #else line can be removed.

243

The same, can be removed.

248

yeah, the comment was attached to the old snapshot. it is the line 254 now ( ASSERT_THAT(Recorded.MacroReferences, Not(IsEmpty()));).

249

The elifndef and elifdef is a C++2b extension feature, so Inputs.ExtraArgs.push_back("-std=c++2b"); to get rid of the [-Wc++2b-extensions] warning in the testcase.

VitaNuo updated this revision to Diff 477721.Nov 24 2022, 3:20 AM

Add -std=c++2b argument to avoid compiler warnings.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
248

Ok,removed the check.

249

Thanks!

VitaNuo updated this revision to Diff 477722.Nov 24 2022, 3:23 AM

Simplify test.

VitaNuo marked 3 inline comments as done.Nov 24 2022, 3:23 AM
hokein accepted this revision.Nov 24 2022, 4:59 AM

Thanks, LGTM. I will commit it for you.

This revision is now accepted and ready to land.Nov 24 2022, 4:59 AM
This revision was automatically updated to reflect the committed changes.