Page MenuHomePhabricator

[TableGen] Add a location for a class definition that was forward-declared
ClosedPublic

Authored by rusyaev-roman on Jul 16 2022, 11:17 AM.

Details

Summary

This change improves ctags generation for tablegen files.

For the following example

class A;

class A {
  int a;
}

Previously, tags were generated only for a forward declaration of class 'A'.

This patch allows generating tags for the forward declarations
and further definition of class 'A'.

Diff Detail

Event Timeline

rusyaev-roman created this revision.Jul 16 2022, 11:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 11:17 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
rusyaev-roman requested review of this revision.Jul 16 2022, 11:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 11:17 AM

changed commit message

rusyaev-roman edited the summary of this revision. (Show Details)Jul 16 2022, 11:43 AM
rusyaev-roman updated this revision to Diff 445248.EditedJul 16 2022, 11:58 AM

Removed unsed 'private' access specifier.

rusyaev-roman edited the summary of this revision. (Show Details)Jul 16 2022, 11:59 AM

This failure is not related to my changes

barannikov88 accepted this revision.Jul 19 2022, 8:49 AM

LGTM, but I can't merge either.

This revision is now accepted and ready to land.Jul 19 2022, 8:49 AM

@silvas @nhaehnle @sunfish Could you take a look at this patch and then merge it please? I have no permission.

Thanks for the patch. Could you please add a test case?

Thanks for the patch. Could you please add a test case?

I've already added the testcase. You can take a look at llvm/test/TableGen/GenTags.td

This revision was landed with ongoing or failed builds.Jul 20 2022, 6:56 AM
This revision was automatically updated to reflect the committed changes.

Right you are, sorry for my confusion. The patch isn't quite right because it overrides the current meaning of the Locs array (which is to track multiclass instantiations), and that can lead to confusion e.g. with PrintMessage. I took the liberty of making some adjustments before committing the patch (394a388d140dc9e74178532501cddb558a589398)

Right you are, sorry for my confusion. The patch isn't quite right because it overrides the current meaning of the Locs array (which is to track multiclass instantiations), and that can lead to confusion e.g. with PrintMessage. I took the liberty of making some adjustments before committing the patch (394a388d140dc9e74178532501cddb558a589398)

I think the original changes cannot lead to confusion in this case. The behavior for multi-class instantiations has not been affected. The original patch has improved tracking for classes, because before these changes, if we have a declaration (or empty definition) and a definition for some class, we can obtain a location only for its forward-declaration. But the original changes provided a location for the definition as well. What do you think?

Right you are, sorry for my confusion. The patch isn't quite right because it overrides the current meaning of the Locs array (which is to track multiclass instantiations), and that can lead to confusion e.g. with PrintMessage. I took the liberty of making some adjustments before committing the patch (394a388d140dc9e74178532501cddb558a589398)

I think the original changes cannot lead to confusion in this case. The behavior for multi-class instantiations has not been affected. The original patch has improved tracking for classes, because before these changes, if we have a declaration (or empty definition) and a definition for some class, we can obtain a location only for its forward-declaration. But the original changes provided a location for the definition as well. What do you think?

My concern here is with the behavior of PrintMessage and friends. The original change would produce misleading output there. WIth the current change, PrintMessage will print the location of the definition (if it is known).

My concern here is with the behavior of PrintMessage and friends. The original change would produce misleading output there. WIth the current change, PrintMessage will print the location of the definition (if it is known).

I see what you mean and it's reasonable. But I think that it's better to hold all locations for the record and decide what to report at a user side. Maybe I'll prepare a related patch in the future, because I feel that introducing 'forward declaration' field is redundant in this case.

Also, I'm going to improve ctags support for tablegen a little bit more in the future.

Anyway, thanks for merging and some suggestions!