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

feat: add scheme start state #2

Merged
merged 5 commits into from
Dec 20, 2022
Merged

feat: add scheme start state #2

merged 5 commits into from
Dec 20, 2022

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Dec 19, 2022

No description provided.

@anonrig anonrig mentioned this pull request Dec 19, 2022
20 tasks
include/ada/url.h Outdated Show resolved Hide resolved
@lemire
Copy link
Member

lemire commented Dec 20, 2022

Random comments which you may consider...

  1. There is currently a mix of different styles. The most common in C++ (at least in the standard library) is all lower-case (except for macros) with underscores: void parse_state(). Whatever one does, we should not mix parseState() and parse_state() in the same code base.
  2. As much as possible, one should try to initialize the variables in the following manner (for example):
parser::parser(const std::string_view input, std::optional<ada::url> optional_base_url, 
   std::optional<ada::encoding_type> encoding_override):  buffer{}, 
   pointer{input.begin()}, encoding{encoding_override.value_or(ada::encoding_type::UTF8)} {
...
}

That is, once you enter the curly bracket part of the constructor, your attributes should have a clear state. I think it is considered form to do the initialization in the curly-bracket component of the constructor.
3. In the header file, you can make sure the attributes of your class are always initialized with the curly-bracket trick:

    std::string_view input{};
    std::string buffer{};
    bool at_sign_seen{}; // will be false
    bool inside_brackets{};
    bool password_token_seen{};
  1. Tools like clang-format will allow one to set a line limit. I think it is often considered bad form to have lines much longer than 80 characters.

@lemire lemire mentioned this pull request Dec 20, 2022
@lemire
Copy link
Member

lemire commented Dec 20, 2022

These are not requests for changes. They are comments.

@anonrig
Copy link
Member Author

anonrig commented Dec 20, 2022

Thank you for the review @lemire. Much appreciated. Let's merge this now, and I'll apply your comments in a separate pull request.

@anonrig anonrig merged commit eee0862 into main Dec 20, 2022
@anonrig anonrig deleted the feat/scheme-start branch December 20, 2022 01:19
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.

2 participants