Page MenuHomePhabricator

[Attributor] Create getter function for the ID of the abstract attribute
ClosedPublic

Authored by bbn on Jul 5 2020, 12:12 AM.

Details

Summary

The getIdAddr() function returns the address of the ID of the abstract attribute

Diff Detail

Event Timeline

bbn created this revision.Jul 5 2020, 12:12 AM
Herald added a reviewer: uenoku. · View Herald Transcript
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
kuter added a comment.EditedJul 5 2020, 1:02 AM

This seems like too much code duplication.
I think we can have a macro to declare getIdAddr and the getName function.
I am curious what other people think about this.

bbn added a comment.Jul 5 2020, 2:37 AM

This seems like too much code duplication.
I think we can have a macro to declare getIdAddr and the getName function.
I am curious what other people think about this.

@kuter Do you mean we can use macro inside the header file? for example, we create such macro and use it inside the declaration of the class, but I still think it is not clean.

#define AA_FUNC_GET_ID(CLASS) \
 const char *getIdAddr() const override { return &ID; }

As far as I know we the getIdAddr() function has to be declared for every AbstractAttributes, so we cannot avoid this. However, maybe we can create a macro for the actual definition of the function (like the createForPosition function)

  • In header file:
struct AA {
  const char *getIdAddr() const override;
  • in source file
#define AA_FUNC_GET_ID(CLASS) \
  const char *CLASS::getIdAddr() { return &ID; }

But I think such getter is short and small, and this way does not seem to reduce the code duplication...

Do you have any other ideas for this? Thanks.

kuter added a comment.EditedJul 5 2020, 2:54 AM
In D83172#2131751, @bbn wrote:

@kuter Do you mean we can use macro inside the header file? for example, we create such macro and use it inside the declaration of the class, but I still think it is not clean.

I was think of having a macro inside the head file.
Attributes are going to be decleared outside of the Attributor soon.

so I think we can have a macro like this:

#define ATTRIBUTE_BOILER(CLASS)                    \
    const char *getIdAddr() { return &ID}                 \
    const std::string getName() { return #CLASS;}   \
    static const char ID;
bbn added a comment.Jul 5 2020, 4:22 AM

Attributes are going to be decleared outside of the Attributor soon.

Do you mean that there might be other abstract attributes defined outside the AttributorAttributes.cpp ?

so I think we can have a macro like this:

#define ATTRIBUTE_BOILER(CLASS)                    \
    const char *getIdAddr() { return &ID}                 \
    const std::string getName() { return #CLASS;}   \
    static const char ID;

Yes this could work, but I am still unsure about it..... I'd also like to hear what others think about this.

In D83172#2131779, @bbn wrote:

Attributes are going to be decleared outside of the Attributor soon.

Do you mean that there might be other abstract attributes defined outside the AttributorAttributes.cpp ?

As of yesterday, there is an AA outside of AttributorAttributes :). You should rebase.

so I think we can have a macro like this:

#define ATTRIBUTE_BOILER(CLASS)                    \
    const char *getIdAddr() { return &ID}                 \
    const std::string getName() { return #CLASS;}   \
    static const char ID;

Yes this could work, but I am still unsure about it..... I'd also like to hear what others think about this.

I mean it is not that much code. Maybe I'm missing something, but why not just use ID directly?

kuter added a comment.Jul 5 2020, 5:09 AM
In D83172#2131779, @bbn wrote:

Attributes are going to be decleared outside of the Attributor soon.

Do you mean that there might be other abstract attributes defined outside the AttributorAttributes.cpp ?

As of yesterday, there is an AA outside of AttributorAttributes :). You should rebase.

so I think we can have a macro like this:

#define ATTRIBUTE_BOILER(CLASS)                    \
    const char *getIdAddr() { return &ID}                 \
    const std::string getName() { return #CLASS;}   \
    static const char ID;

Yes this could work, but I am still unsure about it..... I'd also like to hear what others think about this.

I mean it is not that much code. Maybe I'm missing something, but why not just use ID directly?

You can't access ID when you have a AbstactAttribute&

bbn updated this revision to Diff 275599.EditedJul 6 2020, 1:31 AM

Rebased.

I think we can create a separate patch for the macro stuff.

bbn updated this revision to Diff 277708.Jul 14 2020, 1:49 AM

Added classof function to allow cast from AbstractAttribute to specific AA

jdoerfert accepted this revision.Jul 14 2020, 8:06 AM

LGTM. Merge with the unit test that tests this :)

This revision is now accepted and ready to land.Jul 14 2020, 8:06 AM
This revision was automatically updated to reflect the committed changes.