Comments (7)
As with all things where a large historical codebase is involved, we assume:
-
Things are generally the way they are for a reason, and
-
It's best not to change that unless you:
a) understand the reason it is the way it is,
b) know 100% that the reason is wrong or faulty, and
c) have a fix
In this case, we assume that the logic that was in ASTParser and the logic that was in CompilationUnitResolver and ECJCompilationUnitResolver was generally correct and had a reason to be there.
The previous PR that sparked this discussion attempts to be very careful about merely moving functionality around, but not changing it much at all, as well as formalizing the contract with the caller.
While there's always a push and pull about whether to add something to the interface or not, there are typical concerns that should be weighed against each other.
- Which implementation existed before? Try to stay faithful to that one.
- Is something about the existing implementation dangerous or wrong? A high bar should be present here.
- Should something NOT be added to the interface because it is 100% unnecessary? Variables that no implementer want or desire to use can easily be removed from the interface, but variables where the pre-existing implementation CLEARLY use those variables, and where there is nothing obviously wrong or dangerous about it, should likely be maintained, both to stay faithful to the pre-existing implementation and test cases, as well as to minimize disruption.
If there was something clearly wrong about including the encodings, or if the CompilationUnitResolver clearly wasn't using them, we would have a good argument to remove them from the interface. Since the variables are clearly being used and respected in their implementation at least, if not by callers, we have to fall back to some type of harm calculation.
Removing encodings from the interface has the potential to break any tests that make use of that ability to override the encodings while providing no significant benefit other than the smaller incremental benefit of pruning a codebase of unused features. I would argue the disruption of potentially adding chaos to the test suite and being forced to either disable a number of tests or find some other way to work around the issue points to a greater harm in removing the parameter than in allowing it to remain.
from eclipse.jdt.core.
The encodings in #2560 are used. I'm not sure why they are presumed to be ignored.
A quick trace through some methods shows that the encodings are used.
And here:
From what I can tell, both locations make use of contents = Util.getFileCharContent(new File(sourceUnits[i]), encoding);
I am not sure why @jukzi thinks the encodings are unused.
from eclipse.jdt.core.
My concern is not they are unused but they are used inconsistently.
Both references you gave create a batch.CompilationUnit while for example org.eclipse.jdt.internal.core.CompilationUnit does not know about it
from eclipse.jdt.core.
I think you might be looking at this the wrong way.
The encodings are required so that we know how to read the source file and turn it into a string of characters that we can parse. It is not terribly important that the resulting org.eclipse.jdt.internal.core.CompilationUnit is or is not aware of what encoding it had when it was read from the filesystem. It is more important that the file be read correctly and turned into the jdt.internal.core.CompilationUnit correctly.
The encodings are 100% used (in this PR at least) to read the source files before parsing them and turning them into jdt dom items.
This new interface provides only 5 methods. Only 2 of them require an array of encodings. And these encodings are 100% used in every occasion to read the file from the filesystem. And I would imagine very strongly that anyone extending or using this non-API code would similarly be using the encoding to read the file from the filesystem before parsing it. I know our alternate javac implementation does. The encodings are used, even if they are not part of the returned object.
from eclipse.jdt.core.
ASTParser has a org.eclipse.jdt.core.dom.ASTParser.setEnvironment(String[], String[], String[], boolean)
where someone could setup arbitrary encodings distinct from those used to read a File with Platform.
org.eclipse.jdt.internal.core.CompilationUnit on the other hand reads the content using getResourceContentsAsCharArray() - which does not know about the encoding configured in ASTParser. instead it uses the settings from Platform.
I would like to verify it in the IDE but as far as i see setEnvironment is only used durring tests, so that normally sourcepathsEncodings should be always null.
from eclipse.jdt.core.
from eclipse.jdt.core.
@mickaelistria Is this issue still relevant? Or can it be closed?
from eclipse.jdt.core.
Related Issues (20)
- Various tests are failing on master HEAD
- [Sealed Types] Failure to cast an Object to a generic sealed interface type HOT 5
- Generic inferral is not correctly performed when using wildcard HOT 3
- Comparator Errors HOT 3
- [Sealed Types] Strange error from ECJ: Syntax error on token "permits", permits expected HOT 11
- JDT Core throws ClassCastException: NullTypeBinding cannot be cast to class ArrayBinding HOT 4
- Bug 533327 - [9] Implement JEP 211 HOT 2
- ClasspathMultiReleaseJar no longer finds non module-info class files
- Statically importing a class confuses ECJ HOT 1
- Clean build: SourceFile(s) read twice HOT 20
- In some nestings, autocomplete stops working
- ArrayIndexOutOfBoundsException in org.eclipse.jdt.internal.compiler.parser.Scanner.internalScanIdentifierOrKeyword HOT 5
- [Sealed types] ECJ allows a class to be declared as both sealed and non-sealed HOT 4
- Error in JDT Core during AST creation when using exhaustive switch statement and @NotNull HOT 1
- [Sealed types] Disjointness behavior difference vis a vis javac HOT 2
- [Sealed types + switch expression] Internal inconsistency warning at compile time and verify error at runtime HOT 5
- ClasspathTests / JavaModelTests seem to be unstable HOT 1
- [Switch expression + Sealed Types] Suspect diagnostic about switch expression being inexhaustive HOT 2
- [Sealed Types + Enhanced Switch] Incorrect diagnostic about switch not being exhaustive
- Switch pattern matching accepts invalid case HOT 6
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from eclipse.jdt.core.