clang-tidy modernize-macro-to-enum should check if the #define is ever #undefined

[seanm]

I tried modernize-macro-to-enum on the vtk code base.

The resulting auto fixes result in compile errors.

One is this change:

// turn off 64-bit ints when templating over all types
#undef VTK_USE_INT64
#define VTK_USE_INT64 0
#undef VTK_USE_UINT64
#define VTK_USE_UINT64 0

became:

#undef VTK_USE_INT64
enum {
VTK_USE_INT64 = 0
};
#undef VTK_USE_UINT64
enum {
VTK_USE_UINT64 = 0
};

Perhaps a check could be added where if something is ever #undefed then it should not be transformed to an enum?

[seanm]

@LegalizeAdulthood this may interest you.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[llvmbot]

@llvm/issue-subscribers-bug

[LegalizeAdulthood]

I had some test cases with #undef that should have covered this, but I will take a look at vtk and see if I can reproduce this.

[seanm]

Thanks for your reply.

If you'd rather I try to create a small repro case, let me know.

If you're interested, the diff for what it did against VTK is here

[LegalizeAdulthood]

If you can repro with a small test case, say in compiler explorer, that would be great.

Running clang-tidy on vtk as a whole is also a good idea and I can make a compile_command.json with Ninja

[LegalizeAdulthood]

If you're interested, the diff for what it did against VTK is here

It did pretty well, but I see a few goofs besides the parenthesized literals. Thanks!

[LegalizeAdulthood]

You probably don't want to run clang-tidy on lex/yacc generated files :). I see diffs against vtkParse.tab.c

[LegalizeAdulthood]

per the comments on your gitlab pull request, I'd advise waiting to push until I address the issues you've raised :)

Thanks for the feedback, this is great!

[LegalizeAdulthood]

OK, I see what's happening here. If a macro is #undef'ed or used in a conditional expression and then later defined, then we need to remember a vector of macro names so that if someone attempts to define it later, we exclude it for having been used in an #undef or conditional expression.

We had the reverse case covered -- first defining the macro and then undef'ing it. What's missing is the sequence of preprocessor directives appearing in the opposite order.

[LegalizeAdulthood]

@seanm Did you observe that clang-apply-replacements took a really, really, really, long time? I mean like > 1 hr.

[seanm]

Hmmm, I let it run and did other things. But I did notice that the "applying fixes" stage basically never finished. It applied a bunch of fixes, I left it run overnight, but it still said "applying fixes" this morning.

[LegalizeAdulthood]

OK, yeah, mine's been running clang-apply-replacements for about 5 hours, which is just way too long.

I filed a bug on it.

[LegalizeAdulthood]

Assuming you are using run-clang-tidy.py to process your tree, I'd suggest hacking it up to pass -fix to clang-tidy itself, rather than exporting the fixes and then trying to run clang-apply-replacements.

[LegalizeAdulthood]

I hacked up the python script and it seems to get really hung up on sqlite3.c

So I stripped all "ThirdParty" sources from the vtk compile_commands.json and ran my hacked up script again and it completed in a timely manner.

Honestly, I think it's faster to have clang-tidy apply fixits on the file than it is to have clang-apply-replacements do it, with one exception. The exception is that if you also have header filters enabled. In that case, it's possible that processing more than one source file will identify the same fixits to the same header file. Then you want to generate all the fixits into yaml files and have them reconciled first before being processed with clang-apply-replacements.

sqlite3.c is 235,500 lines long. Clearly the overhead of processing this monster source file is killing whatever algorithm is in clang-apply-replacements.

[LegalizeAdulthood]

@seanm Also, while reviewing your diff in your pull request, I noticed that the changes weren't formatted. You'll want to give -format to run-clang-tidy.py or -format-style=file to clang-tidy so that the changes are formatted according to your .clang-format in your vtk tree.

[LegalizeAdulthood]

@seanm I just noticed that in your draft pull request there were no changes to header files. I think you'll want to run with -header-filter=.*vtk.* or maybe something fancier to exclude the ThirdParty folder.

[LegalizeAdulthood]

@seanm Your code base is really a good test case for this check :)

Bug #54883 describes a scenario I saw in vtkAbstractArray.cxx; it had never occurred to me that people might define macros within a top-level decl!

I will fix this bug first and then address the other bug next.

[LegalizeAdulthood]

@seanm See also bug #54885 which I observed while applying changes with clang-apply-replacements and -header-filters enabled to process vtk headers

[LegalizeAdulthood]

https://reviews.llvm.org/D123889

[seanm]

Thanks @LegalizeAdulthood I will rebuild clang master and give this tool another try on VTK and probably on ITK too!

[LegalizeAdulthood]

@seanm I've got one more fix in flight that you should wait for as it also appears in your code base: Bug #54883