Support identifiers for namespace/template of C++ #83

junjihashimoto · 2019-02-03T02:20:31Z

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 · 2019-03-04T14:47:58Z

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 · 2019-04-12T13:32:10Z

I really want this, too!

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

junjihashimoto · 2019-04-12T13:57:47Z

ok、I try rebuild.

junjihashimoto · 2019-04-12T21:45:23Z

Travis tests have been passed.

bitonic

thanks for the contribution! i'll take a closer look today

missed the tests

bitonic

I'm not sure that parsing template applications into single identifiers is a good idea -- apart from being pretty opaque, it currently doesn't support anything but other identifiers as parameters.

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'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.

What do you think?

junjihashimoto · 2019-04-28T14:05:34Z

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 · 2019-04-29T08:44:14Z

@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 · 2019-05-06T11:41:14Z

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

bitonic · 2019-05-06T11:42:22Z

@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 · 2019-08-05T17:00:54Z

@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 · 2019-08-09T04:21:20Z

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 · 2019-08-13T09:32:48Z

@junjihashimoto thanks! I'll take a look.

bitonic · 2019-08-13T09:58:21Z

@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!

…haskell.org/ghc/ghc/issues/11829)

junjihashimoto · 2019-08-13T14:15:18Z

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

junjihashimoto · 2019-08-13T14:17:47Z

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 · 2019-08-13T14:45:46Z

inline-c-cpp/src/Language/C/Inline/Cpp/Exceptions.hs

@@ -87,7 +87,50 @@ catchBlock = QuasiQuoter
  , quoteDec = unsupported
  } where
      unsupported _ = fail "Unsupported quasiquotation."
-
+
+exceptionalValue :: String -> String


Please comment on what warning this avoids and how you generated this list.

Add the comment.

bitonic · 2019-08-13T14:47:10Z

inline-c-cpp/src/Language/C/Inline/Cpp.hs

@@ -22,6 +26,7 @@ cppCtx :: Context
 cppCtx = baseCtx <> mempty
  { ctxForeignSrcLang = Just TH.LangCxx
  , ctxOutput = Just $ \s -> "extern \"C\" {\n" ++ s ++ "\n}"
+  , ctxEnableCpp = True


Is this necessary? Can't we just use LangCxx to generate this boolean?

It is okay. use langCxx

bitonic · 2019-08-13T14:48:32Z

inline-c/src/Language/C/Types.hs

@@ -100,6 +103,8 @@ data TypeSpecifier
  | TypeName P.CIdentifier
  | Struct P.CIdentifier
  | Enum P.CIdentifier
+  | Template P.CIdentifier [TypeSpecifier]


Please prefix these two cases with Cxx.

bitonic · 2019-08-13T14:53:03Z

inline-c-cpp/test/tests.hs

@@ -1,18 +1,48 @@
-{-# LANGUAGE TemplateHaskell #-}
+{-# LANGUAGE CPP #-}


Pleaste add some tests where the context contains a mapping from a templated type to a haskell type. E.g. * vector<A, B> to something in Haskell.

The mapping of std::vector<Test::Test> is included in the file.

bitonic · 2019-08-13T14:53:14Z

inline-c-cpp/test/tests.hs

@@ -1,18 +1,48 @@
-{-# LANGUAGE TemplateHaskell #-}
+{-# LANGUAGE CPP #-}
+{-# LANGUAGE ConstraintKinds #-}


Are all these extensions really needed? At the very least remove the dangerous ones, such as UndecidableInstances.

No. So I am going to reduce the extensions.

bitonic · 2019-08-13T14:55:40Z

inline-c/src/Language/C/Types.hs

@@ -484,6 +509,8 @@ instance PP.Pretty TypeSpecifier where
    TypeName s -> PP.pretty s
    Struct s -> "struct" <+> PP.pretty s
    Enum s -> "enum" <+> PP.pretty s
+    Template s args -> PP.pretty s <+> "<"  <+>  mconcat (intersperse "," (map PP.pretty args))  <+> ">"


I'd do " >" so that this will work even without C++11.

bitonic · 2019-08-13T14:56:24Z

inline-c/src/Language/C/Types/Parse.hs

@@ -212,13 +215,14 @@ quickCParser typeNames s p = case runCParser typeNames "quickCParser" s p of
 -- | Like 'quickCParser', but uses @'cCParserContext' ('const' 'False')@ as
 -- 'CParserContext'.
 quickCParser_
-  :: String
+  :: Bool


Two things:

To parse namespaces, just change the definition of cpcParseIdent for C++, and export cxxidentifier_no_lex

For templates, put a Bool (say cpcParseCxxTemplates) in the CParserContext.

cCParserContext has The Bool to change the parser for C++.
cxxidentifier_no_lex is not exported, because identifier_no_lex reads and uses CParserContext' value to change the parser.

bitonic · 2019-08-13T15:03:25Z

inline-c/src/Language/C/Types/Parse.hs

@@ -293,11 +298,14 @@ data TypeSpecifier
  | Struct CIdentifier
  | Enum CIdentifier
  | TypeName CIdentifier
+  | Template CIdentifier [TypeSpecifier]
+  | TemplateConst String


Right now this seems to only parse integral const expressions. That's OK, but can you document this, linking to some documentation on what is the full range of const template argument that you can have?

bitonic · 2019-08-13T15:04:28Z

inline-c/src/Language/C/Types/Parse.hs

+templateParser s = parse'
+  where
+    parse' = do
+      id' <- cidentParserWithNamespace


Why do we need this rather than just using cpcParseIdent? That should include namespaces now, right?

It should include namespaces to parse the format like "std::vector".

bitonic · 2019-08-13T15:05:36Z

inline-c/src/Language/C/Types/Parse.hs

+      try (concat <$> sequence [cidentParser, (string "::"), cidentParserWithNamespace]) <|>
+      cidentParser
+    templateArgType = try type_specifier <|> (TemplateConst <$> (many $ oneOf ['0'..'9']))
+    templateArgParser' = do


Please use sepBy templateArgType (symbol ",") or similar.

bitonic · 2019-08-13T15:06:16Z

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

junjihashimoto · 2019-08-14T01:47:48Z

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

junjihashimoto · 2019-08-14T12:08:12Z

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

bitonic · 2019-08-17T20:14:50Z

@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 · 2019-08-19T11:22:06Z

I can wait.

junjihashimoto · 2019-11-22T10:28:35Z

@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 · 2019-11-22T15:24:25Z

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

junjihashimoto · 2019-11-23T02:41:15Z

I'm happy to hear that.

…erType

junjihashimoto · 2019-11-25T07:30:40Z

My modification is done.
This PR works on both linux and macos.
hasktorch/hasktorch#249

bitonic · 2019-11-25T21:38:44Z

@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 · 2019-11-25T21:40:09Z

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

junjihashimoto · 2019-11-26T02:07:42Z

@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 · 2019-11-26T06:53:35Z

@bitonic
Current hackage's inline-c works .
hasktorch/hasktorch#252

junjihashimoto force-pushed the dev branch 2 times, most recently from 43525c9 to a492ef5 Compare February 13, 2019 03:37

junjihashimoto closed this Apr 12, 2019

junjihashimoto reopened this Apr 12, 2019

bitonic previously requested changes Apr 27, 2019

View reviewed changes

bitonic requested changes Apr 28, 2019

View reviewed changes

junjihashimoto mentioned this pull request Jun 11, 2019

a few questions hasktorch/ffi-experimental#44

Closed

junjihashimoto force-pushed the dev branch from eea82af to 6204196 Compare June 17, 2019 01:52

Add bool type

68a0fde

junjihashimoto force-pushed the dev branch from a762221 to fd8f1b0 Compare August 13, 2019 14:06

junjihashimoto and others added 4 commits August 13, 2019 23:11

Fix the test of exception to catch exception on MACOS(https://gitlab.…

db7fac9

…haskell.org/ghc/ghc/issues/11829)

Fix return-value of exception to reduce compiler-warning

6008843

Fix deprecated warning of TH

656f5b6

Support for namespace/template of C++

a7b0ac1

junjihashimoto force-pushed the dev branch from fd8f1b0 to a7b0ac1 Compare August 13, 2019 14:12

Fix CI-error not finding cairo

4a64616

bitonic requested changes Aug 13, 2019

View reviewed changes

Fix reviewing points

7cd10a8

Merge branch 'master' into dev

76261f3

junjihashimoto added 2 commits November 23, 2019 11:59

Add c++-flag(False) of cIdentifierFromString in isGoodHaskellIdentifi…

c607b7e

…erType

Drop unused options of c++

f6c488a

bitonic approved these changes Nov 25, 2019

View reviewed changes

bitonic closed this Nov 25, 2019

bitonic mentioned this pull request Nov 25, 2019

Rethrow Haskell exceptions #95

Closed

		@@ -1,18 +1,48 @@
		{-# LANGUAGE TemplateHaskell #-}
		{-# LANGUAGE CPP #-}

Support identifiers for namespace/template of C++ #83

Support identifiers for namespace/template of C++ #83

Uh oh!

Conversation

junjihashimoto commented Feb 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junjihashimoto commented Mar 4, 2019

Uh oh!

cblp commented Apr 12, 2019

Uh oh!

junjihashimoto commented Apr 12, 2019

Uh oh!

junjihashimoto commented Apr 12, 2019

Uh oh!

bitonic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitonic left a comment

Choose a reason for hiding this comment

Uh oh!

junjihashimoto commented Apr 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bitonic commented Apr 29, 2019

Uh oh!

cblp commented May 6, 2019

Uh oh!

bitonic commented May 6, 2019

Uh oh!

junjihashimoto commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junjihashimoto commented Aug 9, 2019

Uh oh!

bitonic commented Aug 13, 2019

Uh oh!

bitonic commented Aug 13, 2019

Uh oh!

junjihashimoto commented Aug 13, 2019

Uh oh!

junjihashimoto commented Aug 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitonic commented Aug 13, 2019

Uh oh!

junjihashimoto commented Aug 14, 2019

Uh oh!

junjihashimoto commented Feb 3, 2019 •

edited

Loading

bitonic left a comment •

edited

Loading

junjihashimoto commented Apr 28, 2019 •

edited

Loading

junjihashimoto commented Aug 5, 2019 •

edited

Loading

junjihashimoto commented Nov 22, 2019 •

edited

Loading

junjihashimoto commented Nov 26, 2019 •

edited

Loading