Add clang-tidy run on zig sources, minor CMake improvements #993

isaachier · 2018-05-07T11:29:05Z

Upgrade to >= CMake 3.1 (needed for Threads library and other modern CMake features).
Use CMAKE_CURRENT_(SOURCE|BINARY)_DIR instead of CMAKE_(SOURCE|BINARY)_DIR.
Run clang-tidy on Zig C++ sources. (Technically a CMake 3.6 feature but not upgrading minimum CMake just for this.) Can be enabled/disabled with ENABLE_CLANG_TIDY option. Added .clang-tidy file as config for this analysis.

andrewrk · 2018-05-07T21:28:19Z

Thanks for the PR. Did you find anything worth fixing with clang-tidy?

isaachier · 2018-05-07T21:44:24Z

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.

isaachier · 2018-05-07T22:17:39Z

Actually took a look now and see a lot of potential memory leaks in the analyze.cpp file.

isaachier · 2018-05-08T00:54:22Z

Is there a reason the structs in the compiler don't have destructors (i.e. ZigList)?

isaachier · 2018-05-08T01:05:59Z

valgrind confirms this:

$ valgrind --track-origins=yes --leak-check=full ./zig build --build-file ../build.zig --prefix $PWD/stage2 install
==7156== Memcheck, a memory error detector
==7156== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==7156== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==7156== Command: ./zig build --build-file ../build.zig --prefix /home/ihier/proj/zig/zig/build/stage2 install
==7156== 
==7156== Invalid read of size 1
==7156==    at 0xFE4DD5: os_path_join(Buf*, Buf*, Buf*) (os.cpp:229)
==7156==    by 0xFE097F: test_zig_install_prefix(Buf*, Buf*) (main.cpp:160)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156==  Address 0xa670f8f is 1 bytes before a block of size 8 alloc'd
==7156==    at 0x6421ABF: malloc (vg_replace_malloc.c:298)
==7156==    by 0x6423E04: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE434F: buf_init_from_mem(Buf*, char const*, unsigned long) (buffer.hpp:61)
==7156==    by 0xFE440D: buf_init_from_buf(Buf*, Buf*) (buffer.hpp:71)
==7156==    by 0xFE4DB2: os_path_join(Buf*, Buf*, Buf*) (os.cpp:228)
==7156==    by 0xFE097F: test_zig_install_prefix(Buf*, Buf*) (main.cpp:160)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
Unable to find zig lib directory
==7156== 
==7156== HEAP SUMMARY:
==7156==     in use at exit: 96,712 bytes in 1,440 blocks
==7156==   total heap usage: 1,762 allocs, 322 frees, 311,658 bytes allocated
==7156== 
==7156== 64 bytes in 8 blocks are definitely lost in loss record 1,206 of 1,370
==7156==    at 0x6421ABF: malloc (vg_replace_malloc.c:298)
==7156==    by 0x6423E04: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE046D: buf_init_from_mem(Buf*, char const*, unsigned long) (buffer.hpp:61)
==7156==    by 0xFE04E7: buf_init_from_str(Buf*, char const*) (buffer.hpp:67)
==7156==    by 0xFE085D: test_zig_install_prefix(Buf*, Buf*) (main.cpp:144)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 64 bytes in 8 blocks are definitely lost in loss record 1,207 of 1,370
==7156==    at 0x6421ABF: malloc (vg_replace_malloc.c:298)
==7156==    by 0x6423E04: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE046D: buf_init_from_mem(Buf*, char const*, unsigned long) (buffer.hpp:61)
==7156==    by 0xFE04E7: buf_init_from_str(Buf*, char const*) (buffer.hpp:67)
==7156==    by 0xFE0888: test_zig_install_prefix(Buf*, Buf*) (main.cpp:147)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 64 bytes in 8 blocks are definitely lost in loss record 1,208 of 1,370
==7156==    at 0x6421ABF: malloc (vg_replace_malloc.c:298)
==7156==    by 0x6423E04: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE046D: buf_init_from_mem(Buf*, char const*, unsigned long) (buffer.hpp:61)
==7156==    by 0xFE04E7: buf_init_from_str(Buf*, char const*) (buffer.hpp:67)
==7156==    by 0xFE08B3: test_zig_install_prefix(Buf*, Buf*) (main.cpp:150)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 78 bytes in 1 blocks are definitely lost in loss record 1,216 of 1,370
==7156==    at 0x6423DAF: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE4279: buf_resize(Buf*, unsigned long) (buffer.hpp:41)
==7156==    by 0xFE46B4: buf_append_mem(Buf*, char const*, unsigned long) (buffer.hpp:106)
==7156==    by 0xFE4771: buf_append_buf(Buf*, Buf*) (buffer.hpp:118)
==7156==    by 0xFE4E10: os_path_join(Buf*, Buf*, Buf*) (os.cpp:232)
==7156==    by 0xFE09B9: test_zig_install_prefix(Buf*, Buf*) (main.cpp:162)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 78 bytes in 1 blocks are definitely lost in loss record 1,217 of 1,370
==7156==    at 0x6423DAF: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE4279: buf_resize(Buf*, unsigned long) (buffer.hpp:41)
==7156==    by 0xFE46B4: buf_append_mem(Buf*, char const*, unsigned long) (buffer.hpp:106)
==7156==    by 0xFE4771: buf_append_buf(Buf*, Buf*) (buffer.hpp:118)
==7156==    by 0xFE4E10: os_path_join(Buf*, Buf*, Buf*) (os.cpp:232)
==7156==    by 0xFE09D6: test_zig_install_prefix(Buf*, Buf*) (main.cpp:163)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 106 bytes in 2 blocks are definitely lost in loss record 1,349 of 1,370
==7156==    at 0x6423DAF: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE4279: buf_resize(Buf*, unsigned long) (buffer.hpp:41)
==7156==    by 0xFE46B4: buf_append_mem(Buf*, char const*, unsigned long) (buffer.hpp:106)
==7156==    by 0xFE4771: buf_append_buf(Buf*, Buf*) (buffer.hpp:118)
==7156==    by 0xFE4E10: os_path_join(Buf*, Buf*, Buf*) (os.cpp:232)
==7156==    by 0xFE097F: test_zig_install_prefix(Buf*, Buf*) (main.cpp:160)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 134 bytes in 3 blocks are definitely lost in loss record 1,353 of 1,370
==7156==    at 0x6423DAF: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE4279: buf_resize(Buf*, unsigned long) (buffer.hpp:41)
==7156==    by 0xFE46B4: buf_append_mem(Buf*, char const*, unsigned long) (buffer.hpp:106)
==7156==    by 0xFE4771: buf_append_buf(Buf*, Buf*) (buffer.hpp:118)
==7156==    by 0xFE4E10: os_path_join(Buf*, Buf*, Buf*) (os.cpp:232)
==7156==    by 0xFE099C: test_zig_install_prefix(Buf*, Buf*) (main.cpp:161)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 178 bytes in 6 blocks are definitely lost in loss record 1,354 of 1,370
==7156==    at 0x6421ABF: malloc (vg_replace_malloc.c:298)
==7156==    by 0x6423E04: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE434F: buf_init_from_mem(Buf*, char const*, unsigned long) (buffer.hpp:61)
==7156==    by 0xFE440D: buf_init_from_buf(Buf*, Buf*) (buffer.hpp:71)
==7156==    by 0xFE4DB2: os_path_join(Buf*, Buf*, Buf*) (os.cpp:228)
==7156==    by 0xFE097F: test_zig_install_prefix(Buf*, Buf*) (main.cpp:160)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 224 bytes in 8 blocks are definitely lost in loss record 1,359 of 1,370
==7156==    at 0x6421ABF: malloc (vg_replace_malloc.c:298)
==7156==    by 0x6423E04: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE046D: buf_init_from_mem(Buf*, char const*, unsigned long) (buffer.hpp:61)
==7156==    by 0xFE04E7: buf_init_from_str(Buf*, char const*) (buffer.hpp:67)
==7156==    by 0xFE08DE: test_zig_install_prefix(Buf*, Buf*) (main.cpp:153)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 240 bytes in 5 blocks are definitely lost in loss record 1,360 of 1,370
==7156==    at 0x6421ABF: malloc (vg_replace_malloc.c:298)
==7156==    by 0x6423E04: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE434F: buf_init_from_mem(Buf*, char const*, unsigned long) (buffer.hpp:61)
==7156==    by 0xFE440D: buf_init_from_buf(Buf*, Buf*) (buffer.hpp:71)
==7156==    by 0xFE4DB2: os_path_join(Buf*, Buf*, Buf*) (os.cpp:228)
==7156==    by 0xFE099C: test_zig_install_prefix(Buf*, Buf*) (main.cpp:161)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 346 bytes in 7 blocks are definitely lost in loss record 1,362 of 1,370
==7156==    at 0x6421ABF: malloc (vg_replace_malloc.c:298)
==7156==    by 0x6423E04: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE434F: buf_init_from_mem(Buf*, char const*, unsigned long) (buffer.hpp:61)
==7156==    by 0xFE440D: buf_init_from_buf(Buf*, Buf*) (buffer.hpp:71)
==7156==    by 0xFE4DB2: os_path_join(Buf*, Buf*, Buf*) (os.cpp:228)
==7156==    by 0xFE09B9: test_zig_install_prefix(Buf*, Buf*) (main.cpp:162)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 396 bytes in 7 blocks are definitely lost in loss record 1,363 of 1,370
==7156==    at 0x6421ABF: malloc (vg_replace_malloc.c:298)
==7156==    by 0x6423E04: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE434F: buf_init_from_mem(Buf*, char const*, unsigned long) (buffer.hpp:61)
==7156==    by 0xFE440D: buf_init_from_buf(Buf*, Buf*) (buffer.hpp:71)
==7156==    by 0xFE4DB2: os_path_join(Buf*, Buf*, Buf*) (os.cpp:228)
==7156==    by 0xFE09D6: test_zig_install_prefix(Buf*, Buf*) (main.cpp:163)
==7156==    by 0xFE0AD3: find_zig_lib_dir(Buf*) (main.cpp:193)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 438 (216 direct, 222 indirect) bytes in 9 blocks are definitely lost in loss record 1,364 of 1,370
==7156==    at 0x6423BA5: calloc (vg_replace_malloc.c:711)
==7156==    by 0xFE3E34: Buf* allocate<Buf>(unsigned long) (util.hpp:76)
==7156==    by 0xFE03EE: buf_alloc_fixed(unsigned long) (buffer.hpp:46)
==7156==    by 0xFE0419: buf_alloc() (buffer.hpp:52)
==7156==    by 0xFE0A92: find_zig_lib_dir(Buf*) (main.cpp:186)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== 515 bytes in 1 blocks are definitely lost in loss record 1,365 of 1,370
==7156==    at 0x6423DAF: realloc (vg_replace_malloc.c:785)
==7156==    by 0xF5532B: char* reallocate_nonzero<char>(char*, unsigned long, unsigned long) (util.hpp:107)
==7156==    by 0xF56CC8: ZigList<char>::ensure_capacity(unsigned long) (list.hpp:75)
==7156==    by 0xF55A38: ZigList<char>::resize(unsigned long) (list.hpp:58)
==7156==    by 0xFE4279: buf_resize(Buf*, unsigned long) (buffer.hpp:41)
==7156==    by 0xFE5E01: os_self_exe_path(Buf*) (os.cpp:1000)
==7156==    by 0xFE0A77: find_zig_lib_dir(Buf*) (main.cpp:182)
==7156==    by 0xFE0B0D: resolve_zig_lib_dir() (main.cpp:207)
==7156==    by 0xFE1324: main (main.cpp:360)
==7156== 
==7156== LEAK SUMMARY:
==7156==    definitely lost: 2,703 bytes in 74 blocks
==7156==    indirectly lost: 222 bytes in 9 blocks
==7156==      possibly lost: 0 bytes in 0 blocks
==7156==    still reachable: 93,787 bytes in 1,357 blocks
==7156==         suppressed: 0 bytes in 0 blocks
==7156== Reachable blocks (those to which a pointer was found) are not shown.
==7156== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==7156==

andrewrk · 2018-05-08T03:59:53Z

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.

isaachier · 2018-05-08T10:32:51Z

I see. OK let me see if I can change that analysis pass then.

isaachier · 2018-05-09T19:08:11Z

src/parser.cpp

@@ -1037,7 +1037,7 @@ static AstNode *ast_parse_fn_proto_partial(ParseContext *pc, size_t *token_index
        } else if (next_token->id == TokenIdBang) {
            *token_index += 1;
            node->data.fn_proto.auto_err_set = true;
-            next_token = &pc->tokens->at(*token_index);


isaachier · 2018-05-09T19:08:17Z

src/parser.cpp

@@ -1025,7 +1025,7 @@ static AstNode *ast_parse_fn_proto_partial(ParseContext *pc, size_t *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);


isaachier · 2018-05-09T19:08:58Z

src/ir.cpp

@@ -18440,7 +18457,7 @@ static void buf_read_value_bytes(CodeGen *codegen, uint8_t *buf, ConstExprValue
        case TypeTableEntryIdVoid:
            return;
        case TypeTableEntryIdBool:
-            val->data.x_bool = (buf[0] != 0);
+            val->data.x_bool = (buf[0] != 0);  // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult)


Ignore warning for uninitialized value. Can occur if this is within uninitialized array (or at least that is my interpretation.

isaachier · 2018-05-09T19:09:11Z

src/ir.cpp

-            case ConstPtrSpecialFunction:
-                zig_panic("TODO");
+        } else {
+            assert(parent_ptr != nullptr);


Assert parent is not null.

isaachier · 2018-05-09T19:09:29Z

src/ir.cpp

@@ -17433,29 +17446,33 @@ static TypeTableEntry *ir_analyze_instruction_slice(IrAnalyze *ira, IrInstructio
                ptr_val->data.x_ptr.mut = parent_ptr->data.x_ptr.mut;
            }
        } else if (ptr_is_undef) {
+            assert(ptr_val != nullptr);


Assert pointer is not null.

isaachier · 2018-05-09T19:13:25Z

src/ir.cpp

@@ -12027,7 +12030,7 @@ static bool ir_analyze_fn_call_inline_arg(IrAnalyze *ira, AstNode *fn_proto_node

 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,


Need triple pointer to return pointer to allocated memory.

isaachier · 2018-05-09T19:14:00Z

src/ir.cpp

@@ -1116,8 +1117,10 @@ static IrInstruction *ir_build_call(IrBuilder *irb, Scope *scope, AstNode *sourc

    if (fn_ref)
        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);


If arg_count > 0, then args must not be null.

isaachier · 2018-05-09T19:14:20Z

src/codegen.cpp

@@ -5828,20 +5827,20 @@ static const uint8_t int_sizes_in_bits[] = {
 };

 struct CIntTypeInfo {


Optimize struct padding.

isaachier · 2018-05-09T19:14:41Z

src/codegen.cpp

@@ -4306,7 +4306,7 @@ static LLVMValueRef get_coro_alloc_helper_fn_val(CodeGen *g, LLVMTypeRef alloc_f
    LLVMValueRef alloc_fn_val = LLVMGetParam(fn_val, next_arg);
    next_arg += 1;

-    LLVMValueRef stack_trace_val;
+    LLVMValueRef stack_trace_val = nullptr;


Fix issue with using uninitialized value.

isaachier · 2018-05-09T19:14:52Z

src/codegen.cpp

@@ -4317,7 +4317,6 @@ static LLVMValueRef get_coro_alloc_helper_fn_val(CodeGen *g, LLVMTypeRef alloc_f
    LLVMValueRef err_code_ptr = LLVMGetParam(fn_val, next_arg);
    next_arg += 1;
    LLVMValueRef coro_size = LLVMGetParam(fn_val, next_arg);
-    next_arg += 1;


Dead store.

andrewrk · 2018-05-09T20:09:43Z

I think this is largely a distraction.

The C++ codebase only has to build the self hosted compiler
See my specific comments on how clang-tidy is complaining about things that are actually quite intentional and helpful.
The marginal benefits of this static analysis is not worth the code review efforts to make sure these changes are helpful.
It's leading towards little changes that address possible symptoms and not resulting in actual bug fixes.

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.

isaachier · 2018-05-09T20:16:09Z

I definitely understand. It has been interesting exploring this code, but I agree this is more trouble than its worth.

Add clang-tidy run on zig sources, minor CMake improvements

442bf34

isaachier force-pushed the cmake-clang-tidy branch from b359184 to 442bf34 Compare May 7, 2018 11:30

Isaac Hier added 3 commits May 7, 2018 13:02

Upgrade CMake during Mac builds

1fbbec6

Fix typo

0a7062e

Revert fix in Mac, move fix to Linux

1473850

isaachier added 3 commits May 8, 2018 10:14

Fixes from clang-tidy analysis

c72de30

Fix assertions

27c2962

More clang-tidy fixes

f0e77c1

isaachier commented May 9, 2018

View reviewed changes

isaachier closed this May 9, 2018

isaachier deleted the cmake-clang-tidy branch May 9, 2018 20:16

isaachier mentioned this pull request Jun 3, 2018

Fixes for zero-sized allocation #1045

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clang-tidy run on zig sources, minor CMake improvements #993

Add clang-tidy run on zig sources, minor CMake improvements #993

isaachier commented May 7, 2018

andrewrk commented May 7, 2018

isaachier commented May 7, 2018 •

edited

isaachier commented May 7, 2018

isaachier commented May 8, 2018 •

edited

isaachier commented May 8, 2018

andrewrk commented May 8, 2018

isaachier commented May 8, 2018

isaachier May 9, 2018

isaachier May 9, 2018

isaachier May 9, 2018

isaachier May 9, 2018

isaachier May 9, 2018

isaachier May 9, 2018

isaachier May 9, 2018

isaachier May 9, 2018

isaachier May 9, 2018

isaachier May 9, 2018

andrewrk commented May 9, 2018

isaachier commented May 9, 2018

		@@ -5828,20 +5827,20 @@ static const uint8_t int_sizes_in_bits[] = {
		};

		struct CIntTypeInfo {

Add clang-tidy run on zig sources, minor CMake improvements #993

Add clang-tidy run on zig sources, minor CMake improvements #993

Conversation

isaachier commented May 7, 2018

andrewrk commented May 7, 2018

isaachier commented May 7, 2018 • edited

isaachier commented May 7, 2018

isaachier commented May 8, 2018 • edited

isaachier commented May 8, 2018

andrewrk commented May 8, 2018

isaachier commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk commented May 9, 2018

isaachier commented May 9, 2018

isaachier commented May 7, 2018 •

edited

isaachier commented May 8, 2018 •

edited