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

fix: AREAD/AINSERT support in macros called from copybooks #138

Merged
merged 28 commits into from
Jun 23, 2021

Conversation

slavek-kucera
Copy link
Contributor

fix: Address edge cases when AREAD and AINSERT are called from source code brought by COPY instruction.
feat: Add support for macro library to fuzzer.

@slavek-kucera slavek-kucera marked this pull request as ready for review June 22, 2021 08:52
@@ -60,6 +58,13 @@ std::string_view text_data_ref_t::get_range_content(range r) const
return std::string_view(&text->at(start_i), end_i - start_i);
}

std::string_view text_data_ref_t::get_lines_beginning(position pos) const
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more readable for me:

Suggested change
std::string_view text_data_ref_t::get_lines_beginning(position pos) const
std::string_view text_data_ref_t::get_lines_beginning_at(position pos) const

Also why does the function take position instead of just line number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, I wanted get_lines_beginning_at and get_line_beginning_at to have the same signature and did not want to spend time changing the later. So, I'd prefer to keep the position argument for now.

@@ -131,6 +139,10 @@ void opencode_provider::ainsert(const std::string& rec, ainsert_destination dest
m_ainsert_buffer.push_front(rec);
break;
}
// ainsert buffer may contains macro call that will remove code lines from copybooks
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ainsert buffer may contains macro call that will remove code lines from copybooks
// ainsert buffer may contain macro call that will remove code lines from copybooks

{
std::string_view text;
position_t line_no;
const lsp::file_info* copy_file;
Copy link
Contributor

Choose a reason for hiding this comment

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

lsp::file_info is used to hold "LSP metadata" about a file and it seems to me that we do not need all the metadata here, just the newlines. Could this member be changed to const lsp::text_data_ref_t*?

In a future refactoring, we might want to merge some code from file_impl and text_data_ref_t, since both represent the file in a similar way, holding text + positions of newlines.

@@ -81,8 +81,8 @@ class opencode_provider final : public diagnosable_impl, public statement_provid
{
size_t begin_offset;
size_t end_offset;
size_t begin_line;
size_t end_line;
position_t begin_line;
Copy link
Contributor

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 change, since position_t is too similar to position. I know it was not introduced in this PR but maybe I would move in opposite direction - removing the position_t where possible? Or if you think it is useful, rename position_t?

m_suspended = true;
}

bool copy_statement_provider::resume_at(position_t line_no, resume_copy resume_opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool copy_statement_provider::resume_at(position_t line_no, resume_copy resume_opts)
bool copy_statement_provider::try_resume_at(position_t line_no, resume_copy resume_opts)

...since the method checks first?


auto a = std::make_unique<analyzer>(it->second, analyzer_options { library, this, std::move(ctx), data });
a->analyze();
a->collect_diags();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
a->collect_diags();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We end up doing this step during normal processing, so while it is redundant here, I'd prefer keeping it.

@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 9 Code Smells

97.2% 97.2% Coverage
0.0% 0.0% Duplication

@slavek-kucera slavek-kucera merged commit bdc3718 into eclipse-che4z:development Jun 23, 2021
@slavek-kucera slavek-kucera deleted the aread_in_copy branch June 28, 2021 08:09
@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