lahodaj commented on PR #8439:
URL: https://github.com/apache/netbeans/pull/8439#issuecomment-2821429522
Before going into a review in more detail, a few high-level comments:
- the patch seems to contain considerable amount of "debris" - commented out
debugging code (sometimes not commented out debugging code), commented out
code. Preferably, this should be cleaned.
- the patch has considerable amount of "drive-by" changes - changes that are
not directly related to the purpose of the patch, but appear to improve the
code. While some degree of that is OK, and while that may seem desirable, it
complicates the review, and not always brings the value. In the patch, besides
massive whitespace changes (which can, at least, be disabled while viewing the
patch), there are changes like `VeryPretty.visitTopLevel`, where it is unclear
what's the purpose of the changes. I would prefer if the drive-by changes would
be reduced/minimized.
- public classes under `org.netbeans.api.java.source` (and other packages)
are public API. This patch adds two:
- `FlagsMap`: sorry, but surely not for the API, and probably not very
good match at all. This class would require updates on ever javac upgrade, and
also has some other problems (like that the flag numbers mean different things
for different kinds of Symbols - for better or worse). If absolutely needed,
there's `Flags.toString(long)`, which suffers from some issues as well (the
ambiguity in number meanings), but at least does not require updates each time
javac is upgraded.
- `RecordUtils`: while some API to work with records may be in order,
introducing a new API class for them seems excessive. Classes like
`TreeUtilities`/`ElementUtilities`/`TypeUtilities`, and, in some rare cases
`SourceUtils` would be preferred. Also please note that JCTree is *not* an API,
and should not be used in API signatures. (I would also try to limit the newly
added API entry points - it is possible to add new API entry points if needed,
but it is difficult to remove them.)
- please note that new API entry points need `@since`, and an entry in
`<module-name>/apichanges.xml`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
