This is an archive of the discontinued LLVM Phabricator instance.

Tablegen: Remove the error for duplicate include files.
ClosedPublic

Authored by rriddle on Nov 18 2019, 2:53 PM.

Details

Summary

This error was originally added a while(7 years) ago when including multiple files was basically always an error. Tablegen now has preprocessor support, which allows for building nice c/c++ style include guards. With the current error being reported, we unfortunately need to double guard when including files:

  • In user of MyFile.td

#ifndef MYFILE_TD
include MyFile.td
#endif

  • In MyFile.td

#ifndef MYFILE_TD
#define MYFILE_TD
...
#endif

Diff Detail

Event Timeline

rriddle created this revision.Nov 18 2019, 2:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2019, 2:53 PM
rriddle edited the summary of this revision. (Show Details)Nov 18 2019, 2:54 PM

Adding Sean since they added this originally

That is fine with me but I don't have confidence to LGTM this, it'd be nice to get more buy-in!

arsenm added a subscriber: arsenm.Nov 18 2019, 7:12 PM

Needs testcase. TableGen traditionally is lacking many standalone tests and I find it’s an obstacle to development

rriddle updated this revision to Diff 229963.Nov 18 2019, 7:46 PM

Added test case.

Needs testcase. TableGen traditionally is lacking many standalone tests and I find it’s an obstacle to development

Done, thanks for the reminder Matt!

arsenm added inline comments.Nov 18 2019, 9:17 PM
llvm/test/TableGen/duplicate-include.inc
1 ↗(On Diff #229963)

Does this need to be moved into an Inputs directory to avoid lit complaining about the lack of run line?

rriddle marked an inline comment as done.Nov 18 2019, 11:06 PM
rriddle added inline comments.
llvm/test/TableGen/duplicate-include.inc
1 ↗(On Diff #229963)

No, the file suffix is '.inc' which isn't marked in the lit.config as being a test suffix. This also matches the convention of some of the other tests in this directory, examples:

https://github.com/llvm/llvm-project/blob/master/llvm/test/TableGen/prep-diag11-include.inc

https://github.com/llvm/llvm-project/blob/master/llvm/test/TableGen/ConstraintChecking.inc

rriddle updated this revision to Diff 229979.Nov 18 2019, 11:14 PM

Tidy up test.

silvas accepted this revision.Nov 19 2019, 4:59 PM

Thanks. This makes sense now that we have include guards.

This revision is now accepted and ready to land.Nov 19 2019, 4:59 PM
silvas added inline comments.Nov 19 2019, 5:01 PM
llvm/lib/TableGen/Main.cpp
75–77

Make sure this is iterated in a deterministic order.

rriddle updated this revision to Diff 230182.Nov 19 2019, 6:03 PM

Ensure deterministic iteration.

rriddle marked an inline comment as done.Nov 19 2019, 6:03 PM
rriddle marked an inline comment as done.

If there are no other comments, could someone submit this for me?

This revision was automatically updated to reflect the committed changes.