-
Notifications
You must be signed in to change notification settings - Fork 591
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
Fix namespaces and template arguments parsing. #657
Conversation
Other minor fixes.
Please test this on all presets to make sure it doesn't break anything. |
I checked all presets, the PR introduces no changes in the result of parsing but for:
Can we use the CI to check this last 8 presets ? |
Yes, but what we want to check is that the output of the Parser doesn't
change in unexpected ways. We can easily build everything in Docker as per
https://github.com/bytedeco/javacpp-presets/wiki/Build-Environments#linux-x86-and-x86_64
|
I could compile TensorFlow using Docker. It succeeds with master and fails with this PR, but I believe the parser does the right thing now and that the problem is with the preset config file: When parsing
While master produces:
The The JNI from the PR doesn't compile at this line:
with error:
This seems to be related to the definition of the |
This line seems to be missing from ngraph preset. Without it the parsed Java doesn't compile with the PR:
skia doesn't pass the cppbuild, even in docker, error:
Nothing is parsed in Scipy ? The parsing result doesn't differ for the remaining presets: chilitags, tvm, qt, systems. |
Ok, thanks for checking all this. We don't need to worry about TF 1.x and nGraph, those are obsolete and won't have any new releases, so I probably won't be making any for those either. Yes, SciPy doesn't have a native API, that's normal. I guess something changed with the build for Skia, but anyway we're not parsing its C++ API so no need to worry about it. |
Good to merge ? |
Well, instead of having templateThis, templateThat, maybe we could have all that in a Template class just like you did with DocTag? |
We can move static functions |
There's no point in creating a class like ParserUtils... The point is, no other methods use that templatePattern you want to use. So, let's do something about that, like if you want to use more patterns and put them in static variables. |
Thanks! Looks good. Shall we merge? |
Ok for me, you may want to add the missing line in ngraph and have a look at the error for tensorflow, so that all presets can be built. Or are those built already skipped ? |
They won't get built unless they get updated, and they won't get updated. |
At various places, the parser uses simple character searchs like
indexOf('<')
to find templates arguments, or namespace components. This PR replaces this with more robust searchs that should work in all cases like nested templates,operator<
, namespace-qualified template arguments etc...Also add a couple of minor improvements.
More details below.