Skip to content
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

Add token string normalization #1007

Merged
merged 75 commits into from
Apr 25, 2023
Merged

Add token string normalization #1007

merged 75 commits into from
Apr 25, 2023

Conversation

brodmo
Copy link
Contributor

@brodmo brodmo commented Apr 2, 2023

This adds the framework for token string normalization. It also generates semantic information and adds it to the tokens in the Java language module. This cannot be disabled. I couldn't measure a difference in runtime, however.

Copy link
Member

@dfuchss dfuchss left a comment

Choose a reason for hiding this comment

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

Some further comments. Code in general lgtm

@brodmo brodmo requested a review from dfuchss April 19, 2023 10:02
Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

Looks good! Just one conflict to solve, @mbrdl, then I can merge.

# Conflicts:
#	languages/java/src/main/java/de/jplag/java/TokenGeneratingTreeScanner.java
@brodmo brodmo requested a review from tsaglam April 21, 2023 14:30
@dfuchss
Copy link
Member

dfuchss commented Apr 22, 2023

Why did you revert the switch? f3f061e

It should enforce that you have to define behavior for all enum values. (If you don't use default)

@brodmo
Copy link
Contributor Author

brodmo commented Apr 22, 2023

Why did you revert the switch? f3f061e

It should enforce that you have to define behavior for all enum values. (If you don't use default)

But the default is doing nothing? I don't understand, maybe you could provide a code snippet to show me what you mean. I reverted the switch because Sonar didn't like it.

@dfuchss
Copy link
Member

dfuchss commented Apr 22, 2023

Why did you revert the switch? f3f061e

It should enforce that you have to define behavior for all enum values. (If you don't use default)

But the default is doing nothing? I don't understand, maybe you could provide a code snippet to show me what you mean. I reverted the switch because Sonar didn't like it.

I can do that later. The idea would be to use the new switch without default . Then you have to define all cases (therefore, it will cause a compile error if new enum values will be added)

@brodmo
Copy link
Contributor Author

brodmo commented Apr 22, 2023

Why did you revert the switch? f3f061e
It should enforce that you have to define behavior for all enum values. (If you don't use default)

But the default is doing nothing? I don't understand, maybe you could provide a code snippet to show me what you mean. I reverted the switch because Sonar didn't like it.

I can do that later. The idea would be to use the new switch without default . Then you have to define all cases (therefore, it will cause a compile error if new enum values will be added)

From my understanding, you only have to define all cases in a switch expression, not in a switch statement.

        switch (type) {
            case VARIABLE_FLOW -> isVariableFlow = true;
            case VARIABLE_REVERSE_FLOW -> isVariableReverseFlow = true;
        }

compiles just fine, even though more types exist.

@dfuchss
Copy link
Member

dfuchss commented Apr 22, 2023

Why did you revert the switch? f3f061e
It should enforce that you have to define behavior for all enum values. (If you don't use default)

But the default is doing nothing? I don't understand, maybe you could provide a code snippet to show me what you mean. I reverted the switch because Sonar didn't like it.

I can do that later. The idea would be to use the new switch without default . Then you have to define all cases (therefore, it will cause a compile error if new enum values will be added)

From my understanding, you only have to define all cases in a switch expression, not in a switch statement.

        switch (type) {
            case VARIABLE_FLOW -> isVariableFlow = true;
            case VARIABLE_REVERSE_FLOW -> isVariableReverseFlow = true;
        }

compiles just fine, even though more types exist.

I have to check that. There was some kind of switch that enforced that all types are mentioned. As far as I remember.

@dfuchss
Copy link
Member

dfuchss commented Apr 22, 2023

E.g., https://www.happycoders.eu/java/switch-expression/

-> Completeness Analysis

@dfuchss
Copy link
Member

dfuchss commented Apr 22, 2023

But I've recognized in the case where I've marked the code, this does not make sense. So just ignore my initial comment there :)

@dfuchss
Copy link
Member

dfuchss commented Apr 22, 2023

My problem was that I did not realized that you set two different variables based on the enum value.

@brodmo
Copy link
Contributor Author

brodmo commented Apr 24, 2023

Now that I've turned in my thesis, I've decided to add some tests before the presentation on 5/5. Could be good to have them in this PR so we can see the coverage, on the other hand it would be nice to have this PR out of the way. What do you think @tsaglam

@tsaglam
Copy link
Member

tsaglam commented Apr 25, 2023

Now that I've turned in my thesis, I've decided to add some tests before the presentation on 5/5. Could be good to have them in this PR so we can see the coverage, on the other hand it would be nice to have this PR out of the way. What do you think

@mbrdl both is fine with me, whatever you prefer goes. Merging this PR now has the benefit of avoiding conflicts when other PRs are merged.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

89.7% 89.7% Coverage
0.0% 0.0% Duplication

@brodmo
Copy link
Contributor Author

brodmo commented Apr 25, 2023

Test is here, ready to merge 👍 Coverage looking a lot better now 🙃 @tsaglam

@tsaglam tsaglam merged commit 5502e68 into jplag:develop Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue/PR that involves features, improvements and other changes language PR / Issue deals (partly) with new and/or existing languages for JPlag major Major issue/feature/contribution/change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants