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

refactor: Split complex classes #134

Merged
merged 62 commits into from
Jun 11, 2021

Conversation

slavek-kucera
Copy link
Contributor

@slavek-kucera slavek-kucera commented Jun 3, 2021

refactor: parser_impl split
refactor: EOLLN mechanism replaced by true per logical line parsing
fix: apostrophe in strings may be incorrectly reparsed
fix: reparse stops at NUL characters
fix: ignore diags and highlighting info in the lookahead mode
fix: correctly diagnose invalid line continuations
fix: crash on an invalid instruction in the lookahead mode
fix: follow argument restrictions of isspace, isaplha etc.
feat: Lines processed by AREAD are highlighted as such
test: hover over non-existent macro
refactor: unify analyzer options
fix: handle empty variables and concatenations in the label

Copy link
Contributor

@michalbali256 michalbali256 left a comment

Choose a reason for hiding this comment

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

Could you please add comments at least for each class, explaining what the class represent? For example the class logical_line_segment was not immediately clear to me. Maybe even some members/functions would deserve a comment.

parser_library/include/range.h Show resolved Hide resolved
parser_library/src/processing/opencode_provider.cpp Outdated Show resolved Hide resolved
parser_library/src/processing/opencode_provider.h Outdated Show resolved Hide resolved
parser_library/src/processing/opencode_provider.h Outdated Show resolved Hide resolved
@slavek-kucera slavek-kucera marked this pull request as ready for review June 9, 2021 16:27
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

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

92.5% 92.5% Coverage
0.0% 0.0% Duplication

@@ -31,6 +31,13 @@ class parser_error_listener_ctx : public diagnosable_ctx, public parser_error_li
: diagnosable_ctx(hlasm_ctx)
, parser_error_listener_substitution(add_diag_with_ctx, substituted, std::move(provider))
{}
parser_error_listener_ctx(context::hlasm_context& hlasm_ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would I want to use this overload instead of parser_error_listener_substitution? It completely bypasses the added value of context.

Comment on lines +192 to +195
parsing::parser_error_listener_ctx listener(*m_ctx->hlasm_ctx, nullptr);
listener.add_diagnostic(
diagnostic_op::error_E001(range { { line_no, 0 }, { line_no, s.code_offset_utf16 } }));
collect_diags_from_child(listener);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make opencode_provider inherit from diagnosable_ctx? Then these lines would be just:

Suggested change
parsing::parser_error_listener_ctx listener(*m_ctx->hlasm_ctx, nullptr);
listener.add_diagnostic(
diagnostic_op::error_E001(range { { line_no, 0 }, { line_no, s.code_offset_utf16 } }));
collect_diags_from_child(listener);
add_diagnostic(
diagnostic_op::error_E001(range { { line_no, 0 }, { line_no, s.code_offset_utf16 } }));

workspaces::parse_lib_provider& lib_provider,
processing::processing_state_listener& state_listener,
semantics::source_info_processor& src_proc,
const std::string& filename,
Copy link
Contributor

@michalbali256 michalbali256 Jun 11, 2021

Choose a reason for hiding this comment

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

I am not 100% sure about this, but context has method opencode_file_name, probably could replace this parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there appear to be cases when we are passing both the context and the filename as parameters and at the moment I am not 100% convinced that they always match.

@michalbali256 michalbali256 merged commit 78ca7e7 into eclipse-che4z:development Jun 11, 2021
@slavek-kucera slavek-kucera deleted the input-rework branch June 11, 2021 13:49
@github-actions
Copy link

🎉 This PR is included in version 0.14.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 0.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants