Add clang-tidy run on zig sources, minor CMake improvements #993
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
isaachier
force-pushed
the
cmake-clang-tidy
branch
from
b359184 to
442bf34
Compare
|
Thanks for the PR. Did you find anything worth fixing with clang-tidy? |
|
There were a lot of warnings so I was going to go through them for another PR. But I'd be happy to add them here. |
|
Actually took a look now and see a lot of potential memory leaks in the analyze.cpp file. |
|
Is there a reason the structs in the compiler don't have destructors (i.e. |
|
valgrind confirms this: |
|
The c++ compiler treats memory as one big ArenaAllocator. It all gets freed when the compiler exits and finishes the job. We don't bother freeing memory before this. The self-hosted compiler has different requirements - it will be used as a long running process that will perform multiple services, for example compilation every time a file on disk changes, or implementing Language Server Protocol. |
|
I see. OK let me see if I can change that analysis pass then. |
isaachier
commented
May 9, 2018
| } else if (next_token->id == TokenIdBang) { | ||
| *token_index += 1; | ||
| node->data.fn_proto.auto_err_set = true; | ||
| next_token = &pc->tokens->at(*token_index); |
| if (next_token->id == TokenIdKeywordVar) { | ||
| node->data.fn_proto.return_var_token = next_token; | ||
| *token_index += 1; | ||
| next_token = &pc->tokens->at(*token_index); |
| return; | ||
| case TypeTableEntryIdBool: | ||
| val->data.x_bool = (buf[0] != 0); | ||
| val->data.x_bool = (buf[0] != 0); // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) |
There was a problem hiding this comment.
Ignore warning for uninitialized value. Can occur if this is within uninitialized array (or at least that is my interpretation.
| case ConstPtrSpecialFunction: | ||
| zig_panic("TODO"); | ||
| } else { | ||
| assert(parent_ptr != nullptr); |
There was a problem hiding this comment.
Assert parent is not null.
| ptr_val->data.x_ptr.mut = parent_ptr->data.x_ptr.mut; | ||
| } | ||
| } else if (ptr_is_undef) { | ||
| assert(ptr_val != nullptr); |
There was a problem hiding this comment.
Assert pointer is not null.
| static bool ir_analyze_fn_call_generic_arg(IrAnalyze *ira, AstNode *fn_proto_node, | ||
| IrInstruction *arg, Scope **child_scope, size_t *next_proto_i, | ||
| GenericFnTypeId *generic_id, FnTypeId *fn_type_id, IrInstruction **casted_args, | ||
| GenericFnTypeId *generic_id, FnTypeId *fn_type_id, IrInstruction ***casted_args, |
There was a problem hiding this comment.
Need triple pointer to return pointer to allocated memory.
| ir_ref_instruction(fn_ref, irb->current_basic_block); | ||
| for (size_t i = 0; i < arg_count; i += 1) | ||
| for (size_t i = 0; i < arg_count; i += 1) { | ||
| assert(args != nullptr); |
There was a problem hiding this comment.
If arg_count > 0, then args must not be null.
| 128, | ||
| }; | ||
|
|
||
| struct CIntTypeInfo { |
There was a problem hiding this comment.
Optimize struct padding.
| next_arg += 1; | ||
|
|
||
| LLVMValueRef stack_trace_val; | ||
| LLVMValueRef stack_trace_val = nullptr; |
There was a problem hiding this comment.
Fix issue with using uninitialized value.
| LLVMValueRef err_code_ptr = LLVMGetParam(fn_val, next_arg); | ||
| next_arg += 1; | ||
| LLVMValueRef coro_size = LLVMGetParam(fn_val, next_arg); | ||
| next_arg += 1; |
|
I think this is largely a distraction.
I appreciate your efforts @isaachier. I encourage you to redirect your energy toward more rewarding things. There are a lot of cool things to work on - feel free to hop on IRC if you want to discuss some ideas. |
|
I definitely understand. It has been interesting exploring this code, but I agree this is more trouble than its worth. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CMAKE_CURRENT_(SOURCE|BINARY)_DIRinstead ofCMAKE_(SOURCE|BINARY)_DIR.ENABLE_CLANG_TIDYoption. Added.clang-tidyfile as config for this analysis.