This is an archive of the discontinued LLVM Phabricator instance.

Qualify includes of Properties[Enum].inc files. NFC
ClosedPublic

Authored by sammccall on Jul 29 2019, 5:50 AM.

Details

Summary

This is a bit more explicit, and makes it possible to build LLDB without
varying the -I lines per-directory.
(The latter is useful because many build systems only allow this to be
configured per-library, and LLDB is insufficiently layered to be split into
multiple libraries on stricter build systems).

(My comment on D65185 has some more context)

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Jul 29 2019, 5:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2019, 5:50 AM

I am not sure about the consistency argument (on the other CL), as it seems to me that most/all llvm .inc files *which are produced by tblgen" are included by via just their bare names. However, these files generally have unique names, which is usually achieved by just embedding (a derivative of) the folder name into the .inc file name (e.g. AArch64GenAsmWriter.inc, AMDGPUGenAsmWriter.inc, ...). So, I think the most consistent approach here would be to rename lldb files into something like TargetProperties.inc, CoreProperties.inc etc.

Consistency considerations aside, I do think that having a bunch of files called Properties.inc is a bit confusing, and it would be good to disambiguate between them. I'm fairly ambivalent about whether that is achieved by prefixing the include directives with the folder name, or by renaming the files so that they have unique names.

sammccall updated this revision to Diff 212166.Jul 29 2019, 7:48 AM

Give the files distinct names instead.

sammccall updated this revision to Diff 212167.Jul 29 2019, 7:50 AM

Fix one straggler

I am not sure about the consistency argument (on the other CL), as it seems to me that most/all llvm .inc files *which are produced by tblgen" are included by via just their bare names. However, these files generally have unique names, which is usually achieved by just embedding (a derivative of) the folder name into the .inc file name (e.g. AArch64GenAsmWriter.inc, AMDGPUGenAsmWriter.inc, ...). So, I think the most consistent approach here would be to rename lldb files into something like TargetProperties.inc, CoreProperties.inc etc.

Consistency considerations aside, I do think that having a bunch of files called Properties.inc is a bit confusing, and it would be good to disambiguate between them. I'm fairly ambivalent about whether that is achieved by prefixing the include directives with the folder name, or by renaming the files so that they have unique names.

Fair enough, I've given them names that match the main file in that directory (thanks for pointing out this convention!).

Harbormaster completed remote builds in B35753: Diff 212167.
labath accepted this revision.Jul 29 2019, 8:08 AM

Thanks. This looks fine to me, and I think that the new file names fit pretty organically into the existing lldb conventions (as well as being mostly consistent with how llvm handles tablegened files).

This revision is now accepted and ready to land.Jul 29 2019, 8:08 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2019, 10:23 AM

Committed this for you as r367241, including a rebase past r367238 (which I really hope I didn't mess up -- ninja check-lldb passed, at least).

lldb/trunk/source/Target/Process.cpp