Repository navigation
[BUG] General code style issues #177
Description
In digging thought the code while debugging my usage of it, I've noticed a number of oddities that seem like things that could easily be cleaned up to improve the code readability.
Would PR's doing general cleaning be welcomed and actively supported?
In general, I think the sort of cleanup described below would improve the code by both making it more readable as well as by making it shorter (in a few cases, by a lot), but it would also be rather invasive in that it would likely touch a significant faction of the code which could cause merge conflicts and the like. That said, most everything I've see would be highly local and simple so any merge conflicts would likely be relatively trivial.
Examples:
- Inconsistent formatting:
The formatting style is rather non-uniform (e.g. if() and if () are both used, function argument sometimes skip spaces after the comma, most lines are under 80col but a few hundred are not, etc.). With tools like clang-format, it should be a rather mechanical task to identify the most prevalent patterns and apply them globally.
- Pre/post C++11 idioms:
The code uses post C++11 features like std::move() but also makes extensive uses of patterns rendered obsolete by C++11, in particular; iterator for loops rather than range for.
- Others:
- redundant lookups:
if (m.count(k)) return m[k];vs
auto i = m.get(); if (i != m.end()) return i->second;) - redundant constructor definitions (e.g. where
= defaultwould have the same effect) - redundant actions in destructor (clearing a
std::mapthat is about to it self be destructed). - etc.
- redundant lookups:
These are just the ones I've seen so far and I suspect if I dig, I'd find more:
Activity
etr commentedon Feb 17, 2020
Hey,
thanks for offering to help. The lib is showing signs of its age and the stratification of refactors (e.g. introducing C++11, etc.) is glaring.
I'd be happy to review and merge any PR that addresses any of the stylistic issues you mentioned above.
Closing this issue as I guess was more of a general question - feel free to re-open if I haven't fully addressed your ask.
bcsgh commentedon Feb 18, 2020
Part of the reason I opened this bug was as a persisted public place to discuss things. In digging a bit more, it seems that Gitter is what you use for that? However 1) that appears to be yet another chat app with 2) yet another account and 3) it has no activity in the last 4 months ... so I'm not going to use it.
Do you have a suggestion of a better place to discuss things? A mailing list (like what google groups provides) would be ideal.
In the meantime, a few questions:
Is there a reason that http_response::content isn't included in the copy/move constructor/assignment operators?
http_request::content_size_limit is of type size_t (which is unsigned) but initialized to -1. This seems like an issue. It might be better to explicitly use std::numeric_limits::max() as that seems to be the actual effect?
is there a reason http_request::unescaper is not handled consistently by the copy/move constructors/assignment?
etr commentedon Feb 18, 2020
Happy to leave this open if it is helpful to drive a conversation.
I never set-up a mailing list as the community of the library has been relatively small till recent times (that's also why you see gitter being scattered).
On the specifics of your questions:
Use default constructor and operator= where possible: etr#177
bcsgh commentedon Feb 24, 2020
Question: In
httpserver::details::modded_request, shouldcallbackanddhrsbe included in the copy/move semantics? Right now they are not.etr commentedon Feb 26, 2020
There is no reason for not copying/moving them.
Rename some variable so that all `this->` can be dropped, and do so. (#…
fechantillac-antidot commentedon Jul 9, 2020
What about integrating tools like clang-tidy, clang-format as part of the build process ?