Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cpp] Support for all the enums #7

Open
chandangreddy opened this issue Jul 25, 2017 · 6 comments
Open

[cpp] Support for all the enums #7

chandangreddy opened this issue Jul 25, 2017 · 6 comments

Comments

@chandangreddy
Copy link

Currently, only isl::stat is supported. We can follow the same strategy as isl::stat and all enums to isl.h.top and generate appropriate static_casts while generating methods. We need to decide on names of enums as the simple rule of removing type name from enums will result in C++ default keywords for some like isl_ast_node_for, isl_ast_node_if etc..

@tobiasgrosser
Copy link
Member

Yes, would be great to support these. I assume that Sven would prefer to auto-generate the enums. We have already a translation table for cases where names conflict with C++ keywords. We can likely reuse this here.

@tobiasgrosser
Copy link
Member

@chandangreddy : Any plans to move this forward. I think it would be great to see some of the isl ast interface to be exposed.

@chandangreddy
Copy link
Author

@TobiG Yes, I have modified the generator to handle all enums uniformly. It basically strips isl_ from the name of enums and does static_cast whenever necessary. But I assume that all the enum classes are defined manually in the isl.h.top. I think, it not worth the effort to generate enum definitions automatically, given the limited number of enums that we have. Could you please share the translation tables that is agreed upon?
I can send PR for handling enums with their definitions in isl.h.top.

@tobiasgrosser
Copy link
Member

@chandangreddy : Thanks for looking into this. I am slightly confused what you mean. You say you modified the generator, but prefer to add manual declarations of enums to isl.h.top? This seems to be inconsistent?

I personally believe for small enums which are unlikely to ever be extended we can indeed just copy the declaration to isl.h.top. isl_dim is such a case. Maybe we should start with this and a couple of related functions?

Larger enums (such as the ones in the AST generator) are likely better auto-generated.

The translation table is part of interface/cpp.cc:

/* An array listing functions that must be renamed and the function name they    
 * should be renamed to. We currently rename functions in case their name would  
 * match a reserved C++ keyword, which is not allowed in C++.                    
 */                                                                              
static const char *rename_map[][2] = {                                           
        { "union", "unite" },                                                    
};   

and is still mostly empty.

@chandangreddy
Copy link
Author

chandangreddy commented Aug 2, 2017

@TobiG The generator modifications are to generate static_casts in the methods like

enum isl::ast_op_type ast_expr::get_op_type() const {
  auto res = isl_ast_expr_get_op_type(get());
  return static_cast<enum isl::ast_op_type>(res);
}

isl::multi_pw_aff multi_pw_aff::add_dims(enum isl::dim_type type, unsigned int n) const {
  auto res = isl_multi_pw_aff_add_dims(copy(), static_cast<enum isl_dim_type>(type), n);
  return manage(res);
}

The enum declarations are manually added to isl.h.top.
My question was did you guys had discussion regarding translation for other C++ keywords like for, id, and, or etc...?

@tobiasgrosser
Copy link
Member

Thanks, now i understand your use of the generator.

No, I don't think we had a discussion here. You may have a look at these two patches:

simbuerg/isl@1a95bcc
simbuerg/isl@4be4dd0
simbuerg/isl@a052050

for how enums in general can be added to the generator.

However, to spark the discussion a minimal patch with the enum in isl.h.top and the small code generator changes might be a good start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants