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

554 var in out strings returned from functions are truncated to length 81 #568

Conversation

mhasel
Copy link
Member

@mhasel mhasel commented Sep 7, 2022

Fixes generic function annotation for string generics to allow for string lengths > 80. It is now also possible to pass long string literals as function parameters without saving them into a variable first.
Fixes Issue #554

@mhasel mhasel linked an issue Sep 7, 2022 that may be closed by this pull request
@mhasel mhasel closed this Sep 7, 2022
@mhasel mhasel reopened this Sep 8, 2022
@mhasel mhasel marked this pull request as draft September 8, 2022 06:45
@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2022

Codecov Report

Base: 93.76% // Head: 93.81% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (49d78e2) compared to base (35842fa).
Patch coverage: 91.60% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #568      +/-   ##
==========================================
+ Coverage   93.76%   93.81%   +0.05%     
==========================================
  Files          46       46              
  Lines       17604    17661      +57     
==========================================
+ Hits        16506    16569      +63     
+ Misses       1098     1092       -6     
Impacted Files Coverage Δ
src/codegen/generators/expression_generator.rs 88.84% <85.18%> (-0.25%) ⬇️
src/resolver/generics.rs 96.51% <93.22%> (-0.61%) ⬇️
src/parser.rs 96.72% <100.00%> (+0.45%) ⬆️
src/resolver.rs 97.54% <100.00%> (ø)
src/index/visitor.rs 96.40% <0.00%> (-0.16%) ⬇️
src/diagnostics.rs 81.61% <0.00%> (+0.70%) ⬆️
src/codegen/generators/llvm.rs 89.84% <0.00%> (+1.01%) ⬆️
src/validation.rs 98.32% <0.00%> (+1.39%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…r string parameters that are shorter than they were declared as
…54-var_in_out-strings-returned-from-functions-are-truncated-to-length-81
… had the name 'placeholder'.

Removes overly convoluted test.
updates generic resolver tests. changes string type annotation for literals. adds requested comments to previous changes.
…ngs-returned-from-functions-are-truncated-to-length-81
…from-functions-are-truncated-to-length-81' into 554-var_in_out-strings-returned-from-functions-are-truncated-to-length-81
@mhasel mhasel marked this pull request as ready for review September 23, 2022 08:53
@mhasel mhasel requested a review from riederm September 23, 2022 08:54
@ghaith ghaith linked an issue Sep 27, 2022 that may be closed by this pull request
src/index.rs Outdated
@@ -1395,6 +1395,18 @@ impl Index {
} => self
.find_effective_type_info(referenced_type)
.unwrap_or(initial_type),
DataTypeInformation::String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a bit surprising for me.
is this always right? - also if you think about the contract of the method?
why is the intrinsic of STRING[10] == STRING[80]?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would feel better if this special case gets handled somewhere in the generics.rs

call_name: Some(call_name),
},
)
let annotation = self
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets rename and document this function
better name: get_specific_function_annotation and in the parameters ask for: generic_qualified_name &generic_return_type

takes the generic signature of a function generic function and resolves it to its specific types according to the generic_map and the generic_name_resolver.

@@ -73,8 +73,6 @@ impl<'i> TypeAnnotator<'i> {
..
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment: make clear that this information describes the generic function

Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

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

overall looks good, I added 2 or 3 small remarks

@mhasel
Copy link
Member Author

mhasel commented Nov 3, 2022

due to the changes from #605, string varargs are now passed as double pointers. this will be fixed in a separate issue.

@mhasel mhasel requested a review from riederm November 3, 2022 14:30
Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

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

lets undo the set_is_call lambda thing
resolver.rs:1295 self.visit_statement(&ctx.set_is_call(), operator);

@mhasel
Copy link
Member Author

mhasel commented Nov 10, 2022

Added the failing test from #635. The generated snap looks good to me and I think the issue can be closed as soon as this PR is approved and merged.

@ghaith ghaith linked an issue Nov 10, 2022 that may be closed by this pull request
…ions-are-truncated-to-length-81

fixes merge conflicts. adds allow dead code pragma to unused function in tests.rs - maybe it should be removed instead.
@mhasel mhasel requested a review from riederm November 10, 2022 10:11
Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

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

👍 good work

@ghaith ghaith merged commit 34a47fc into master Nov 14, 2022
@ghaith ghaith deleted the 554-var_in_out-strings-returned-from-functions-are-truncated-to-length-81 branch November 14, 2022 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants