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 gcc build warning #101

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

kumajaya
Copy link

No description provided.

src/core/datatypes/forte_string.cpp Outdated Show resolved Hide resolved
src/core/datatypes/forte_string.cpp Outdated Show resolved Hide resolved
Move the switch case into an own method to make the code much more
readable
default:
DEVLOG_ERROR("Unknown escape symbol %c encountered", inputString[i + 1]);
if (getSpecialSymbol(inputString[i + 1], specialSymbol) != -1) {
unescapedString += specialSymbol; // append escaped char

Choose a reason for hiding this comment

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

As mentioned before, shouldn't we abort with an error here? @azoitl
Now we are just not adding the invalid escaped symbol, thereby silently ignoring an error, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

True I missed that. However, giving it a second thought I'm not perfectly sure what should happen here. I had a look in IEC 61131-3 and there nothing is stated about invalid $ escapings. Does anyone know what other languages are doing. Should we just ignore the $ then. I think this is what happens in C or?

Choose a reason for hiding this comment

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

The compiler gives you an error what I have seen on StackOverflow

Copy link
Author

Choose a reason for hiding this comment

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

The previous logic possibly add uninitialized specialSymbol char that trigger compiler warning. I'm also not sure what should happen here.

Choose a reason for hiding this comment

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

Don't get me wrong, it is an improvement. I am. Just not sure if we should create a valid string in such a case

Copy link
Contributor

Choose a reason for hiding this comment

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

@kumajaya first of all thx for your patience with us. With your work you are definitely improving 4diac FORTE. But as you are poking on a rather important component and as you are changing it, we think it would be good that we get it right this time.

Copy link
Author

Choose a reason for hiding this comment

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

@azoitl No problem, sir! I found this great project and started loving it from the beginning because it seemed to suit my needs. Just a little help from my side, I always take compiler warnings seriously and try to understand why they occur but of course I always need good guidance from people who understand more here.

Choose a reason for hiding this comment

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

@kumajaya it's good you fix compiler warnings, it is important and I fixed a bunch myself, but not always high on our priorities ;)

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.

3 participants