Conversation
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
lizan
requested review from
PiotrSikora,
alyssawilk,
dio,
htuch,
jmarantz,
mattklein123,
snowp and
zuercher
as code owners
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
Looks good with one question about Chromium source.
| template <typename T> class CanonOutputT { | ||
| public: | ||
| CanonOutputT() : buffer_(NULL), buffer_len_(0), cur_len_(0) {} | ||
| CanonOutputT() : buffer_(NULL) {} |
There was a problem hiding this comment.
s/NULL/nullptr/ and also make this an initializer like *_len_ -- but only if we are going to continue to apply clang-tidy to this Chromium code.
|
|
||
| #include <stdlib.h> | ||
| #include <string.h> | ||
| #include <cstdlib> |
There was a problem hiding this comment.
higher level question for this file: this is obviously not written for Envoy, but appears to be forked from Chromium, I'm assuming because depending on its upstream was not tractable.
But rather than bringing it into our coding style, should we add a third_party directory where we hold forked sources, and do not apply our current clang-tidy standards or naming conventions?
There's at least a few other Envoy naming-style divergences in this file as well, but it doesn't seem to make sense to fix them.
In fact all of the changes suggested here might make it harder to manually pull from Chromium later on if we decide we need to.
There was a problem hiding this comment.
+1 can we exclude this code?
There was a problem hiding this comment.
FWIW, we're not far away from being able to use the GURL library directly for this thanks to #10599. So, if we can easily skip, that could simplify.
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
with the chromium edits gone, lgtm & thanks!
|
/azp run envoy-presubmit |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I think you might need a master merge since stats integration test is failing. /wait |
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
|
This actually saves memory per cluster. I think it is because of the use of |
Nice! |
|
@lizan there are still tidy errors but I'm going to go ahead and merge this to avoid further creep. We can continue to do follow ups. Thank you! |
|
@mattklein123 thanks. tidy errors are expected as I'm not fixing |
4 participants
Signed-off-by: Lizan Zhou lizan@tetrate.io
Description:
Fix clang-tidy error on master. Except
readability-identifier-namingwhich causes too many function rename.Risk Level: Low
Testing: CI
Docs Changes: N/A
Release Notes: N/A