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

Update the CTRE library #388

Merged
merged 4 commits into from May 4, 2021
Merged

Update the CTRE library #388

merged 4 commits into from May 4, 2021

Conversation

joka921
Copy link
Member

@joka921 joka921 commented May 3, 2021

Updated the CTRE library to the current release. The most visible effect of this is getting rid of compiler warnings.

It also includes small adaptations of the CTRE-based Turtle Parser, because the internal semantics of the ctll::fixed_string have slightly changed.

Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

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

Thanks + I assume that you knew what you where doing concerning the length of a fixed_string (counting the trailing zero towards the length or not)

Comment on lines +19 to +29
constexpr ctll::fixed_string<A + B> operator+(const ctll::fixed_string<A>& a,
const ctll::fixed_string<B>& b) {
char32_t comb[A + B + 1] = {};
for (size_t i = 0; i < A; ++i) { // omit the trailing 0 of the first string
comb[i] = a.content[i];
}
for (size_t i = 0; i < B; ++i) {
comb[i + A - 1] = b.content[i];
comb[i + A] = b.content[i];
}
// the input array must be zero-terminated
comb[A + B] = '\0';
Copy link
Member

Choose a reason for hiding this comment

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

  1. Was the 0 part of the string length before and now it isn't?
  2. Do we use ++i consistently instead of i++ ? I am aware that there is a difference between the two, when the value is used. Is there a difference when it is not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Yes exactly, in the new ctre version "a" becomes fixed_string<1> (previously fixed_string<2>). We have exhaustive tests that would horribly break if this method was wrong.

  2. If the value is used, then post- vs. pre-increment makes a huge difference, as you have correctly stated (has been like this "forever" in C. When the value is not used, then ++i is never less efficient than i++. For builtin types, like int etc. it is exactly the same. But if the incremented object i is e.g. an iterator to a complex data structure, which stores several pointers etc., then i++ has to create a copy of the iterator (we need to old value to possibly still use it, and the new value, because we requested an increment. For this reason, I have started to use ++i as a default in for loops (of course only where range-based for loops don't apply.

Comment on lines +128 to +129
fixed_string("[\\+\\-]?([0-9]+\\.[0-9]*") + ExponentString + "|" +
ExponentString + ")";
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the explicit fixed_string instead of the abbreviation fs.

Is fs still used elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

the fs is not used, and already is deleted in the code you are looking at.

@joka921 joka921 merged commit 6db499e into ad-freiburg:master May 4, 2021
@joka921 joka921 deleted the f.updateCtre branch May 4, 2021 07:11
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.

None yet

2 participants