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

Bug on parinsing trivial YAML #413

Closed
1 of 2 tasks
ebertolazzi opened this issue Oct 18, 2024 · 12 comments
Closed
1 of 2 tasks

Bug on parinsing trivial YAML #413

ebertolazzi opened this issue Oct 18, 2024 · 12 comments
Labels
bug Something isn't working

Comments

@ebertolazzi
Copy link

Description

The parse failed to parse a single item

Reproduction steps

If you parse the following YAML

b: 1e-1

I obtain the error:

Failed to convert a scalar to an integer. (at line 0, column 3)

If you use

b: 0.1e-2

all is ok.

It seems that the parser try to force an integer for number in engineering form that start with a integer.

Expected vs. actual results

nope

Minimal code example

No response

Error messages

No response

Compiler and operating system

clang MAC OS

Library version

0.3.13

Validation

@ebertolazzi
Copy link
Author

This workaround

    case node_type::INTEGER: {
        integer_type integer = 0;
        bool converted = detail::atoi(token.begin(), token.end(), integer);
        //@@@@@@@@@@@@@@@@@@ WORKAROUND @@@@@@@@@@@@@@@@@@@@@
        if ( !converted ) {
          float_number_type float_val = 0;
          bool converted = detail::atof(token.begin(), token.end(), float_val);
          if FK_YAML_UNLIKELY (!converted) {
            throw parse_error("Failed to convert a scalar to an integer or float.", m_line, m_indent);
          }
          node = basic_node_type(float_val);
        } else {
          node = basic_node_type(integer);
        }
        break;
    }

in

basic_node_type create_scalar_node(node_type type, str_view token)

permit to parse the file but it shuild be better to change

node_type decide_value_type(lexical_token_t lex_type, tag_t tag_type, str_view token) const noexcept

in order to detect the correct type.

@fktn-k
Copy link
Owner

fktn-k commented Oct 18, 2024

@ebertolazzi
Thanks for filing the issue and suggesting workaround.
I agree with your opinion that the decide_value_type method should be changed.
I guess the root cause would be in the scalar_scanner class, which has concrete implementation for detecting scalar node value type from string.
I'll investigate the issue and try to figure out a fix.

@fktn-k fktn-k added the bug Something isn't working label Oct 19, 2024
@fktn-k
Copy link
Owner

fktn-k commented Oct 19, 2024

@ebertolazzi
I've found the root cause in the scalar_scanner class and, as you mentioned, it tries to force a scalar to be an integer when the significand is an integer.
Also, some other floating point number formats related to the decimal point and/or the exponent, are turned out to not be parsed correctly too, so I'm fixing them too.
I'll let you know when all the fixes get available.

@fktn-k
Copy link
Owner

fktn-k commented Oct 20, 2024

@ebertolazzi
The issue should be fixed in the PR #414.
Could you confirm with the develop HEAD?

@ebertolazzi
Copy link
Author

Now work well! Thanks a lot for your excellent work.

I want to signal an annoying warning with the NEW c++ compiler:

node.hpp:12811:32: warning: identifier '_yaml' preceded by whitespace in a literal operator declaration is deprecated [-Wdeprecated-literal-operator]
12811 | inline fkyaml::node operator"" _yaml(const char32_t* s, std::size_t n) {
| ~~~~~~~~~~~^~~~~
| operator""_yaml

it seems that the standard now prefer to remove white space in definition of literal operator.

regards,

@fktn-k
Copy link
Owner

fktn-k commented Oct 20, 2024

@ebertolazzi
Thanks for your confirmation!

it seems that the standard now prefer to remove white space in definition of literal operator.

I didn't know that, thanks for the information.
Such a whitespace seems deprecated in C++23 (CWG-2521) and new compilers emit a deprecation warning against that.
As far as I know, however, such a whitespace is required in C++11 and that's why the declarations have it in between.
So, it seems some kind of preprocessor switch is necessary which depends on the active C++ standard.

@fktn-k
Copy link
Owner

fktn-k commented Oct 26, 2024

@ebertolazzi
I'm having trouble in reproducing the "-Wdeprecated-literal-operator" warning even if the active C++ standard is C++23.
I've tested the following compilers on Ubuntu 24.04 LTS:

  • g++ 13.2.0
  • g++ 14.2.0
  • clang++ 18.1.3

I know it's so annoying to see a number of warnings you cannot resolve without somehow ignoring them, and so I want to fix them.
Could you provide some more information? Compiler version, operation system, and compile options would be helpful.

@ebertolazzi
Copy link
Author

to stop message it is enough to use a pragma

#ifdef clang
#pragma clang diagnostic ignored "-Wdeprecated-literal-operator"
#endif

@ebertolazzi
Copy link
Author

The compiler is

clang -v ─╯
Apple clang version 16.0.0 (clang-1600.0.26.3)
Target: x86_64-apple-darwin24.0.0

on OSX sequoia 15.0.1

@fktn-k
Copy link
Owner

fktn-k commented Oct 26, 2024

@ebertolazzi
Thanks for your timely response!
I could finally reproduce it. The -Wdeprecated compile option is necessary for reproduction.
Anyway, I'm disabling the warning by using a pragma as you suggested.

@fktn-k
Copy link
Owner

fktn-k commented Oct 27, 2024

@ebertolazzi
The warning should be disabled by the PR #417. Could you confirm with the develop HEAD again?

@fktn-k
Copy link
Owner

fktn-k commented Nov 16, 2024

I'll close this issue. The fixes are included in the release v0.3.14. If any related issue is found, feel free to reopen it.

@fktn-k fktn-k closed this as completed Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants