VitaNuo added inline comments.
================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:95
private:
- void recordMacroRef(const Token &Tok, const MacroInfo &MI) {
+ void recordMacroRef(const Token &Tok, const MacroInfo &MI, RefType RT) {
if (MI.isBuiltinMacro())
----------------
hokein wrote:
> 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.
Right, didn't know default parameter values were possible in C++, thanks.
================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:103
private:
- void recordMacroRef(const Token &Tok, const MacroInfo &MI) {
+ void recordMacroRef(const Token &Tok, const MacroInfo &MI, RefType RT =
RefType::Explicit) {
if (MI.isBuiltinMacro())
----------------
hokein wrote:
> this line exceeds the max 80 character limit, clang-format should fix it.
Formatting should be fine now.
================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:228
+ llvm::Annotations MainFile(R"cpp(
+ #include "header.h"
+
----------------
hokein wrote:
> 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)
>
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.
================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:233
+ #ifdef ^X
+ #define Y 2
+ #endif
----------------
hokein wrote:
> the `#define Y 2` can be removed, our test doesn't care about it. just use
>
> ```
> #ifdef X
> #endif
> ```
> is enough.
Ok, I'll remove all the fluff.
================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:255
+ EXPECT_EQ(Ref.RT, RefType::Ambiguous);
+ EXPECT_EQ(RefNames[I], Ref.Target.macro().Name->getName());
+ RefOffsets.push_back(Off);
----------------
hokein wrote:
> 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());`.
>
>
Ok, I've reduced everything to X.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138559/new/
https://reviews.llvm.org/D138559
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
