Skip to content

Conversation

@eric-milles
Copy link
Member

This is not complete, but does give some shape to recent discussions.

  • ASM_API_VERSION goes to AsmClassGenerator
  • DEFAULT_TARGET_BYTECODE and defaultTargetBytecode() were unnecessary additions
  • setTargetBytecode(String) can use nearest value as described in GROOVY-10278
  • isPostJDKn can be replaced with use of isAtLeast(String,String) -- the recent additions could be dropped, but I just moved them for now
  • LoggableClassVisitor --> TraceClassVisitor (just need to work out the jarjar stuff for unit tests

Thoughts?

return cv;
}
return new LoggableClassVisitor(cv, config);
private static ClassVisitor createClassVisitor(final ClassVisitor visitor, final CompilerConfiguration config) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnusedVariable: The parameter 'config' is never read. (details)
(at-me in a reply with help or ignore)

@daniellansun
Copy link
Contributor

LoggableClassVisitor --> TraceClassVisitor

After reading tons of "minor edits" and this PR, I am still not sure why "inline" or "anonymous" code is preferred over code with meaningful names, e.g. method names, class names, etc.

-1 from me.

@eric-milles
Copy link
Member Author

eric-milles commented Dec 7, 2021

Such as what? If you gave some examples, I might be able to provide reasoning. The purpose of the draft was to discuss these items rather than just be dismissive.

What is the concern with leveraging TraceClassVisitor which is provided instead of maintaining LoggableClassVisitor which tries to do the same job and I've found many problems with the flushing and the multiple instance creation?

If you are referencing the replacement of isPostJDK5(), then my raised concerns earlier are that "is post" is not the correct semantic. It is really "is at least" and the use if isAtLeast(targetBytecode,JDK5) is conveys that semantic and does not require a new public API method for every Java release, which are coming every 6 months now, not every few years. Again, this is demonstrated for discussion purposes. It is not a must have.

I disagree with lifting one-time use values up to the top of a class file. I disagree with adding public API for something that might be useful but has no track record of need. These are specific examples that can foster further discussion.

@eric-milles
Copy link
Member Author

Now if the new methods added were isAtLeastJDK9 and so on and isPostJDK5, etc. were removed or scheduled for removal, then my only concern would be the need to add a new method every six months. That kind of work should be automated IMO.

@paulk-asert
Copy link
Contributor

According to a strict interpretation of ASF rules, a -1 on a code submission is a veto:
https://www.apache.org/foundation/voting.html#votes-on-code-modification
I'll close this issue and see if there are any non-controversial parts which can be cherry-picked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants