Support identifiers for namespace/template of C++

[junjihashimoto]

This PR supports identifiers for namespace/template of C++. We want to use libtorch library which uses various Tensor classes(at::Tensor, torch::Tensor, caffe2::Tensor and so on).

I think cIdentLetter should support c types only and this modification is hackey. But the implementation of identifier is not pluggable.

Could you merge this PR?

[junjihashimoto]

Though cIdent parser of first commit is hackey, cppIdentParser and its test are added on last commit. cppIdentParser checks the correspondence of template-parentheses. cpp-parser has little influence on c-parser.

[cblp]

I really want this, too!

Could you please retry Travis tests? It seems to be a temporary network problem.

[junjihashimoto]

ok、I try rebuild.

[junjihashimoto]

Travis tests have been passed.

[junjihashimoto]

Thank you for your reply. I appreciate your reviewing.

There's also the problem that this addition is now present for all uses of inline-c, although I'm a bit less bothered by that.

I think so. I'm sorry to trouble you. But I do not have good idea how to separate c-parser and c++-parser. How about putting a flag changing the parsers somewhere? Do you have any suggestions?

I'd be happier with extending data Type i with a CppTemplate P.CIdentifier [Type i], and then implementing the proper C++ grammar for template applications.

This is good. I will try it. But c++-template arguments support just number, e.g. std::array<bool,4>. The "4" is not "data Type i" probably.

[bitonic]

@junjihashimoto oh, nothing to be sorry about! I think it's a very nice feature to add, we just need to add it right.

And sorry about how long it took me to review this.

But c++-template arguments support just number, e.g. std::array<bool,4>. The "4" is not "data Type i" probably.

Yes, we have to extend our types a bit. I'd be pretty conservative, probably adding only numeric literals, since those are very useful in C++.

[cblp]

Is something bad in adding something that can be a type in C++ but not in C?

[bitonic]

@cblp no, but the way it is added in the current pr is extremely ad-hoc -- only identifiers can be arguments of templates, and that property is not even checked.

[junjihashimoto]

@bitonic long time no see.

I have extended data TypeSpecifier instead of data Type i. Because template-type is a bit similar to struct-type in data TypeSpecifier, and we can reduce the code to parse it. Then the parser generates CppTemplate Ary '(CInt,10)-type from array<int,10> of c++-type. How about this PR?

data TypeSpecifier
  = Void
  | Bool
  | Char (Maybe Sign)
  | Short Sign
  | Int Sign
  | Long Sign
  | LLong Sign
  | Float
  | Double
  | LDouble
  | TypeName P.CIdentifier
  | Struct P.CIdentifier
  | Enum P.CIdentifier
  | Template P.CIdentifier [TypeSpecifier]
  | TemplateConst String
  deriving (Typeable, Show, Eq, Ord)

[junjihashimoto]

Current implementation improves mapping of template-type. e.g. std::array<int,4> of C++'s template-type is mapped to Array '(Int,4). And it has a switch to use c-parser or c++-parser.

[bitonic]

@junjihashimoto thanks! I'll take a look.

[bitonic]

@junjihashimoto First of all, the branch in this PR seems to contain a lot of unrelated changes. Could you isolate the changes related to the template arguments, or at the very least have them so that I can easily review commit-by-commit? Thanks!

[junjihashimoto]

@bitonic Sorry for including the changes and thank you for reading it. I have isolated the commit related to the template argument.

[junjihashimoto]

This pr includes following items.

• Support for namespace/template of C++
• Fix deprecated warning of TH
• Fix return-value of exception to reduce compiler-warning
• Fix the test of exception to catch exception on MACOS(https://gitlab.haskell.org/ghc/ghc/issues/11829)
• Add bool type

[bitonic]

@junjihashimoto did a first review pass. Overall the approach looks good, thanks a lot for the contribution!

[junjihashimoto]

@bitonic Thank you for reviewing and good advice. I am going to update the pr soon.

[junjihashimoto]

I think the modification is done and there may be some differences from your expectations.

[bitonic]

@junjihashimoto just wanted to let you know that I have not forgotten about this -- but I've been a bit busy and I'm not entirely sure that the template parsing is safe. Specifically I'm not sure whether we need to store the C++ type names too. I'll try to check and merge this soon.

[junjihashimoto]

I can wait.

[junjihashimoto]

@bitonic Hi, long time no see. Please review before the new year, if you can do it. Because we want to release hasktorch and upload it before the new year. 🙇 I will fix CI-error on the weekend. If you think we should fork this pr or do other thing to support c++, let me know.

[bitonic]

@junjihashimoto I'm very sorry about the delay, I'll try to review it in the next few days.

[junjihashimoto]

I'm happy to hear that.

[junjihashimoto]

My modification is done. This PR works on both linux and macos. https://github.com/hasktorch/hasktorch/pull/249

[bitonic]

@junjihashimoto I have rebased and merged your PR. Again, deep apologies about the delay, I got really busy and this slipped off my radar. Moreover, it wasn't an obvious merge -- I'm still not totally convinced about the new grammar. But it's a useful feature, and I think it's better to have it.

Thank you so much for the change and the patience.

[bitonic]

Released now as inline-c 0.9.0.0 and inline-c-cpp 0.3.1.0.

[junjihashimoto]

@bitonic Thank you for your supporting and comments. If this pr is not obvious for you, so other person thinks so too. I should improve documentions and add examples.

[junjihashimoto]

@bitonic Current hackage's inline-c works . https://github.com/hasktorch/hasktorch/pull/252