Skip to content

Added parse_ini_string, parse_ini_file functions#690

Closed
pmswga wants to merge 5 commits intoVKCOM:masterfrom
pmswga:pmswga/ini_parsing
Closed

Added parse_ini_string, parse_ini_file functions#690
pmswga wants to merge 5 commits intoVKCOM:masterfrom
pmswga:pmswga/ini_parsing

Conversation

@pmswga
Copy link
Copy Markdown

@pmswga pmswga commented Nov 28, 2022

At the moment, the functions do not process an array of values.

Example:

[Section3.1]
var1[]=val_1_1
var1[]=val_1_2
var1[]=val_1_3

This section will skip.

@tolk-vm tolk-vm requested a review from DrDet November 28, 2022 14:24
Copy link
Copy Markdown
Contributor

@quasilyte quasilyte left a comment

Choose a reason for hiding this comment

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

This is a first review round. I'm sure @DrDet will find something else here, but let's start with something simple.

Comment thread runtime/parsing_functions.cpp Outdated
Comment thread runtime/parsing_functions.cpp Outdated
Comment thread runtime/parsing_functions.cpp Outdated
Comment thread runtime/parsing_functions.cpp Outdated
Comment thread tests/cpp/runtime/parsing-functions-test.cpp Outdated
Copy link
Copy Markdown
Contributor

@DrDet DrDet left a comment

Choose a reason for hiding this comment

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

In general, I'm not sure it's correct way of parsing ini string. For example, PHP use autogenerated parser for that. Can you explain your parsing method somehow?

Comment thread runtime/parsing_functions.cpp Outdated
Comment thread runtime/parsing_functions.h Outdated
Comment thread runtime/parsing_functions.cpp Outdated
Comment thread runtime/parsing_functions.cpp Outdated
Comment thread runtime/parsing_functions.h Outdated
@pmswga
Copy link
Copy Markdown
Author

pmswga commented Nov 29, 2022

general, I'm not sure it's correct way of parsing ini string. For example, PHP use autogenerated parser for that. Can you explain your parsing method somehow?

I didn't use the automatically generated parsers, because:

  1. I didn't think about it
  2. I'm not sure that I could correctly implement it in runtime

Therefore, the solution that came to my mind is to check the resulting strings by regular expressions.

Copy link
Copy Markdown

@Tsygankov-Slava Tsygankov-Slava left a comment

Choose a reason for hiding this comment

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

I have found several places where something can be improved. It would be great if you took these comments into attention.

Comment thread runtime/parsing_functions.cpp Outdated
Comment thread runtime/parsing_functions.cpp Outdated
Comment thread runtime/parsing_functions.cpp Outdated
Comment thread runtime/parsing_functions.cpp Outdated
- removed all std utils
- removed all extra functions
- code refactoring
Comment thread runtime/ini.cpp
Comment on lines +7 to +9
const int64_t SINGLE_QOUTES = 0;
const int64_t DOUBLE_QUOTES = 1;
const int64_t ALL_QUOTES = 2;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In this case, it's better to use enum.

enum qoutes_type {
    SINGLE_QOUTES,
    DOUBLE_QUOTES,
    ALL_QUOTES
} qoutes_type_t;

Comment thread runtime/ini.cpp
const int64_t DOUBLE_QUOTES = 1;
const int64_t ALL_QUOTES = 2;

string clear_quotes(const string &str, int64_t flag) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The variable named flag contains little information about what exactly is there, I think it's better to call this variable quotes_type.

Then the function declaration will look like this:

string clear_quotes(const string &str, qoutes_type_t quotes_type);

Comment thread runtime/ini.cpp
Comment thread runtime/ini.cpp
Comment on lines +42 to +44
Optional<string> clear_str;

clear_str = f$preg_replace(string("/ +/"), string(" "), str);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In this case, it's better to take advantage of the opportunities that C++11 standard gives us and use auto.

auto clear_str = f$preg_replace(string("/ +/"), string(" "), str);

Comment thread runtime/ini.cpp
Comment thread runtime/ini.cpp
Comment thread runtime/ini.cpp
Comment thread runtime/ini.cpp
Comment thread runtime/ini.cpp
string ini_entry;
string ini_section;

for (string::size_type i = 0; i <= ini_string_copy.size(); ++i) {
Copy link
Copy Markdown

@Tsygankov-Slava Tsygankov-Slava Dec 3, 2022

Choose a reason for hiding this comment

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

Firstly, also it's better to use auto here.
Secondly, it looks very strange that your for goes to ini_string_copy.size(), suddenly there will be a situation that you turn to ini_string_copy[ini_string_copy.size()]) (for example, if a string of the form [text somehow gets to your function). It's not very good.
It's better to fix on

for (string::size_type i = 0; i < ini_string_copy.size(); ++i) {
...
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Type of i is deduced as int, it is not an intention.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Type of i is deduced as int, it is not an intention.

Thanks, you're right.
Fixed

Comment thread runtime/ini.cpp
@tolk-vm tolk-vm added the missing functionality Missing PHP functions, classes, constants and etc label Mar 9, 2023
@pmswga pmswga closed this by deleting the head repository May 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

missing functionality Missing PHP functions, classes, constants and etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants