bazel/clang-tidy: Cleanups and optimizations #2496
Conversation
Signed-off-by: Ryan Northey <ryan@synca.io>
✅ Deploy Preview for nifty-bassi-e26446 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
cc @mathetake |
| permitted_file_types = [ | ||
| ".c", ".cc", ".cpp", ".cxx", ".c++", ".C", ".h", ".hh", ".hpp", ".hxx", ".inc", ".inl", ".H", | ||
| ] | ||
| for file_type in permitted_file_types: | ||
| if src.basename.endswith(file_type): | ||
| return True |
There was a problem hiding this comment.
can we use dict here? for loop with more than 10 items seems a bit unnecessary linear search
There was a problem hiding this comment.
not sure i follow exactly - in real python there are various ways to optimize this but i dont think they work in starlark - can you give example?
There was a problem hiding this comment.
PERMITTED_TYPES = {"c": True, "cpp": True}
def perimtted(file_name):
if file_name.split(".")[-1] in PERMITTED_TYPES:
return True
like this?
There was a problem hiding this comment.
not sure if the dict would be faster than a list
what i think we can do is ...
| permitted_file_types = [ | |
| ".c", ".cc", ".cpp", ".cxx", ".c++", ".C", ".h", ".hh", ".hpp", ".hxx", ".inc", ".inl", ".H", | |
| ] | |
| for file_type in permitted_file_types: | |
| if src.basename.endswith(file_type): | |
| return True | |
| permitted_file_types = [ | |
| ".c", ".cc", ".cpp", ".cxx", ".c++", ".C", ".h", ".hh", ".hpp", ".hxx", ".inc", ".inl", ".H", | |
| ] | |
| return src.extension in permitted_file_types |
There was a problem hiding this comment.
probably - we can shift the list out and get rid of the fun also
There was a problem hiding this comment.
at least can you move .h and .cc to the first elements?
There was a problem hiding this comment.
they are the most common in envoy code base
There was a problem hiding this comment.
re .h i think we dont want it to run over the headers directly it causes a stack of other issues
if we do want that then we need to fish them out from the hdrs attr
| # echo "RUN: ${CLANG_TIDY_BIN} ${*}" | ||
|
|
There was a problem hiding this comment.
i figured keep it - its kinda useful for debugging
| @@ -158,11 +137,11 @@ def _clang_tidy_aspect_impl(target, ctx): | |||
| src, | |||
| target.label.name, | |||
| ) | |||
| for src in srcs | |||
| for src in _rule_sources(ctx) | |||
There was a problem hiding this comment.
I think this outputs can be one run i guess? i don't see any reason why we cannot run clang-tidy for all source codes in the target at once vs here run each file per clang-tidy.
There was a problem hiding this comment.
so the one issue with this plan is that the flags are set according to whether src is .c or .cc
a quick grep of the code suggests there are not any builds that have both as a src (not sure that would make sense - but what do i know) - so probably surmountable
on the upsides it makes the code cleaner and removes the need to use to_list on the srcs
There was a problem hiding this comment.
i believe all Envoy codes are C++ hence .cc and .c are for test C program code (not clang-tidy worthy), so we can simply ignore them
There was a problem hiding this comment.
so next problem is
38 outfile = ctx.actions.declare_file(
39 "bazel_clang_tidy_%s.%s.clang-tidy.yaml" % (infile.path, discriminator)
40 )i can workaround for now - i think in the original implementation this was intended for the fix file altho not sure it was actually implemented
my intention is to use this output for the output from clang-tidy and then collect those up so probs we can think of a different path scheme
There was a problem hiding this comment.
lets deal with this separately - the benefit is marginal at best and it requires quite a lot of change to make it work
Signed-off-by: Ryan Northey <ryan@synca.io>
No description provided.