Skip to main content
added 201 characters in body
Source Link
lubgr
  • 956
  • 5
  • 12
  • Why does it have to be a static data member s_bInComment? I assume removeComment is static, too? This sounds like it's somewhat decoupled from the rest of the Parser class, which might have some instance-specific state. Maybe it makes things cleaner and more testable if you create a small class only for the removal of comments, which improves encapsulation and circumvents the static variable - non-const, static variables are too global for my taste, and they are generally thread-unsafe.

  • The function is very long, and it contains many nested branches. I think it's common sense that this is not very readable and should be split into smaller functions. If you give them meaningful names, you can even remove some of your comments (e.g. hasPartialEndToken(str) or startsWithGlobalToken(str)).

  • If you care about allocations and C++17 is available, you could change the way this functions passes its result back to the caller, i.e., via a std::vector<std::string_view>. The lifetime of the input argument is controlled by the caller, so it is probably sufficient to not copy the non-comment parts of the string into a result, but instead create a collection of views on the input. As std::string::substr and concatenating strings with operator + are possibly expensive operations, you might keep this option in mind (and of course, only go with it after you made sure that performance/memory footprint is important, you profiled your code and so on...)

  • Minor point, as this is not in your list of tested string: what should happen with something like "a /* b \n c */ d # e", i.e., a multiline comment followed by a until-the-end-of-line comment?

  • Why does it have to be a static data member s_bInComment? I assume removeComment is static, too? This sounds like it's somewhat decoupled from the rest of the Parser class, which might have some instance-specific state. Maybe it makes things cleaner and more testable if you create a small class only for the removal of comments, which improves encapsulation and circumvents the static variable - non-const, static variables are too global for my taste, and they are generally thread-unsafe.

  • The function is very long, and it contains many nested branches. I think it's common sense that this is not very readable and should be split into smaller functions. If you give them meaningful names, you can even remove some of your comments (e.g. hasPartialEndToken(str) or startsWithGlobalToken(str)).

  • If you care about allocations and C++17 is available, you could change the way this functions passes its result back to the caller, i.e., via a std::vector<std::string_view>. The lifetime of the input argument is controlled by the caller, so it is probably sufficient to not copy the non-comment parts of the string into a result, but instead create a collection of views on the input. As std::string::substr and concatenating strings with operator + are possibly expensive operations, you might keep this option in mind (and of course, only go with it after you made sure that performance/memory footprint is important, you profiled your code and so on...)

  • Why does it have to be a static data member s_bInComment? I assume removeComment is static, too? This sounds like it's somewhat decoupled from the rest of the Parser class, which might have some instance-specific state. Maybe it makes things cleaner and more testable if you create a small class only for the removal of comments, which improves encapsulation and circumvents the static variable - non-const, static variables are too global for my taste, and they are generally thread-unsafe.

  • The function is very long, and it contains many nested branches. I think it's common sense that this is not very readable and should be split into smaller functions. If you give them meaningful names, you can even remove some of your comments (e.g. hasPartialEndToken(str) or startsWithGlobalToken(str)).

  • If you care about allocations and C++17 is available, you could change the way this functions passes its result back to the caller, i.e., via a std::vector<std::string_view>. The lifetime of the input argument is controlled by the caller, so it is probably sufficient to not copy the non-comment parts of the string into a result, but instead create a collection of views on the input. As std::string::substr and concatenating strings with operator + are possibly expensive operations, you might keep this option in mind (and of course, only go with it after you made sure that performance/memory footprint is important, you profiled your code and so on...)

  • Minor point, as this is not in your list of tested string: what should happen with something like "a /* b \n c */ d # e", i.e., a multiline comment followed by a until-the-end-of-line comment?

Source Link
lubgr
  • 956
  • 5
  • 12

That's an interesting case. First, I thought that the function should be a function template for a pair of input iterators and an output iterator, as that's an idiomatic approach taken by many STL algorithms and what the function matches this pattern. But given that \n plays a crucial role here, passing iterators to an underlying std::ifstream doesn't make much sense (it consumes words, not lines).

Nevertheless, let me share my thoughts besides.

  • Why does it have to be a static data member s_bInComment? I assume removeComment is static, too? This sounds like it's somewhat decoupled from the rest of the Parser class, which might have some instance-specific state. Maybe it makes things cleaner and more testable if you create a small class only for the removal of comments, which improves encapsulation and circumvents the static variable - non-const, static variables are too global for my taste, and they are generally thread-unsafe.

  • The function is very long, and it contains many nested branches. I think it's common sense that this is not very readable and should be split into smaller functions. If you give them meaningful names, you can even remove some of your comments (e.g. hasPartialEndToken(str) or startsWithGlobalToken(str)).

  • If you care about allocations and C++17 is available, you could change the way this functions passes its result back to the caller, i.e., via a std::vector<std::string_view>. The lifetime of the input argument is controlled by the caller, so it is probably sufficient to not copy the non-comment parts of the string into a result, but instead create a collection of views on the input. As std::string::substr and concatenating strings with operator + are possibly expensive operations, you might keep this option in mind (and of course, only go with it after you made sure that performance/memory footprint is important, you profiled your code and so on...)

Last, a more general question - why aren't you using <regex>? The API is not famous for its ease of use, but you do a lot of manual work that could be outsourced to the standard library, and developers maintaining such a piece of code wouldn't be surprised to find some regular expressions in it. Here's a small class has the same functionality as the snippet you posted:

class CommentRemover {
    public:
        std::string strip(const std::string& input);

    private:
        bool isInComment = false;
        static const std::regex restOfLine;
        static const std::regex partialInline;
        static const std::regex multiLineBegin;
        static const std::regex multiLineEnd;
};

with the actual std::regex objects being defined as

const std::regex CommentRemover::restOfLine{"(//|#)"};
const std::regex CommentRemover::partialInline{"(/\\*.*?\\*/)"};
const std::regex CommentRemover::multiLineBegin{"(/\\*.*(?!\\*/))"};
const std::regex CommentRemover::multiLineEnd{"(/\\*.*(?!\\*/))"};

and the implementation of the member function looks like

std::string CommentRemover::strip(const std::string& input)
{
    std::smatch match;
    // Save some typing below:
    const auto search = [&match, &input](const std::regex& re){
        return std::regex_search(input, match, re); };

    if (isInComment && search(multiLineEnd)) {
        isInComment = false;
        return strip(match.suffix().str());
    } else if (search(partialInline))
        return match.prefix().str() + strip(match.suffix().str());
    else if (search(restOfLine))
        return match.prefix().str();
    else if (search(multiLineBegin)) {
        isInComment = true;
        return match.prefix().str();
    }

    return input;
}

This is not a prime example for readability, and the regex specifications are admittedly cryptic. But I think it reduces the likelihood of bugs due to less code and no manual looping/indexing. Plus, it allows for concentrating the complexity of the operation into the regex definitions and the if-else if ordering.