Fix compilation errors with Eclipse compiler#13532
Merged
martint merged 1 commit intotrinodb:masterfrom Sep 19, 2022
Merged
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
findepi
reviewed
Aug 8, 2022
Member
There was a problem hiding this comment.
This is a correct simplification.
Was the original code an incorrect Java, according to JLS?
Member
Author
There was a problem hiding this comment.
The compiler complained about the usage:
The method upcast(TupleDomain<T>) in the type TupleDomain<T> is not applicable for the arguments (capture#20-of ? extends TupleDomain<? extends T>)
so it couldn't infer the U type, I'm not sure, if ECJ is too limited, or Javac is more lazy in this case.
Member
There was a problem hiding this comment.
I am fine with this particular change. In the past, I've seen ECJ having problems where it shouldn't. I don't have any recent experience with this compiler.
Is there any reason you want to use ECJ?
Member
Author
There was a problem hiding this comment.
Yes, because this is what Eclipse is using,and a bit annoying that I can't start the app, without fixing the errors 😉
Member
There was a problem hiding this comment.
Isn't it possible to configure Eclipse to use javac as a compiler?
Member
|
Re package names -- airlift/airbase#321 (it looks like it's valid Java to have |
findepi
reviewed
Aug 8, 2022
Member
There was a problem hiding this comment.
Why?
The class has no subclasses. Did you mean to make it package private?
Member
Author
There was a problem hiding this comment.
Yes, package private would work as well.
findepi
changed the title
gzsombor
force-pushed
the
compilation-errors
branch
from
154e062 to
d3d771b
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
Member
|
Let's structure this in the following commits I am not sure what to do about Also, when using Eclipse, do you have a choice which compiler to build with? E.g. can you delegate the build to maven / javac? (BTW we recommend IntelliJ for development) |
Member
Author
|
I think, there are a couple of issues around the visibility rules in ECJ and Javac, and in this case, ECJ has the less problem. But, this is only a compilation error in ECJ - Javac is inconsistent Interestingly, this works in both : however, if we write this: this fails in ECJ - which definitely looks bad. |
gzsombor
force-pushed
the
compilation-errors
branch
from
d3d771b to
a4bc00a
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
gzsombor
force-pushed
the
compilation-errors
branch
from
a4bc00a to
6d02453
Compare
gzsombor
force-pushed
the
compilation-errors
branch
from
6d02453 to
6c5e83f
Compare
Member
Author
|
For me, it's unclear why these tests are failing, I suspect this is independent from the changes. |
Member
|
It indeed looks unrelated and is a flaky test. Click to see full stack trace |
Member
Author
|
Thanks! |
hashhar
reviewed
Aug 23, 2022
Member
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
In "Fix package declaration for TestingRedirectHandlerInjector":
TestingRedirectHandlerInjector is placed in correct folder - just the package declaration is wrong. So instead of changing the path to the file the first commit should just change package io.trino.jdbc to package io.trino.tests.product.jdbc.
Looks good other than that.
gzsombor
force-pushed
the
compilation-errors
branch
2 times, most recently
from
26e866e to
48b2f43
Compare
Member
|
@gzsombor In case you're still pushing can you also do following:
|
martint
requested changes
Aug 23, 2022
Member
martint
left a comment
martint
left a comment
There was a problem hiding this comment.
Some of the changes in this PR are ok and stand on their own, but I don't think we should make changes solely for the purpose of appeasing the Eclipse compiler. It is not a goal of Trino to support compilers that don't follow the JLS closely, and we can't guarantee that certain changes won't inadvertently be rolled back or new incompatibilities introduced in the future.
Member
There was a problem hiding this comment.
If I'm reading the JLS correctly, this seems to be a bug in the Eclipse compiler:
A member (class, interface, field, or method) of a class, interface, type parameter,
or reference type, or a constructor of a class, is accessible only if (i) the class, interface, type parameter, or reference type is accessible, and (ii) the member or
constructor is declared to permit access:
[...]
– Otherwise, the member or constructor is declared private. Access is
permitted only when the access occurs from within the body of the top level
class or interface that encloses the declaration of the member or constructor.
and
The body of a class declares members (fields, methods, classes, and interfaces),
instance and static initializers, and constructors (§8.1.7).
Member
There was a problem hiding this comment.
Isn't it possible to configure Eclipse to use javac as a compiler?
gzsombor
force-pushed
the
compilation-errors
branch
from
48b2f43 to
8707da9
Compare
Member
Author
|
Yes, you are right, the constant visibility problem is more of an eclipse bug - it seems that in the Java 6 era, Javac had a similar bug, and this is when this logic was introduced, to be compatible with that old Javac. |
This is equivalent and shorter. This also fixes compilation problem when compiling with ECJ.
gzsombor
force-pushed
the
compilation-errors
branch
from
8707da9 to
fe2676d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The Eclipse compiler seems to be more picky about things, like the package declaration and the places of the file, etc.
This patch tries to address these issues.
Related issues, pull requests, and links
Documentation
( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
( ) No release notes entries required.
( ) Release notes entries required with the following suggested text: