[Lldb-commits] [PATCH] D15593: Enhance "target modules dump line <file>" and use it to fix MI's -symbol-list-lines.

[Jim Ingham via lldb-commits]

> On Dec 17, 2015, at 10:25 AM, Dawn Perchik <dawn+llvm at burble.org> wrote:
> 
> dawn marked 6 inline comments as done.
> dawn added a comment.
> 
>> clang-format your changes please (there are many deviations from the coding style)
> 
> 
> I've made some fixes even though the code is no longer consistent with the rest of the file.  Alas.  I would love to run all of lldb through clang-format, but as we've seen, there are several options which still need to be added before we can do that (mostly relating to the formatting function declarations).
> 
> 
> ================
> Comment at: source/Commands/CommandObjectTarget.cpp:1510
> @@ -1503,2 +1509,3 @@
> 
> -        for (uint32_t i=0; i<num_matches; ++i)
> +            for (uint32_t i=0; i<num_matches; ++i)
> +            {
> ----------------
> ki.stfu wrote:
>> nit: fix indentation
> This is the original code, and is consistent with the coding style of the rest of the file.  It bothers me too that lldb is so schizophrenic about its coding style, but unless we fix all of lldb, I think it's best to just try and follow the style of the code around you.

lldb is not "schizophrenic about its coding style."  The rules about nesting are quite clear, for instance.  There are just places where people have made mistakes, for instance where somebody added an extra level of nesting and forgot to indent the contained code.  Running clang-format on new submissions - then hand checking the result to make sure it hasn't done something incorrect - is a good idea.  And if you see errors while reading around, fix them as separate checkins.

> 
> Does anyone else have an opinion about this?  I'll go ahead and change it, only because I see I that the new code I added didn't follow this style (oops).
> 
> ================
> Comment at: source/Commands/CommandObjectTarget.cpp:1541-1542
> @@ +1540,4 @@
> +                CompUnitSP cu_sp(module->GetCompileUnitAtIndex(i));
> +                if (!cu_sp)
> +                    continue;
> +                CompileUnit *cu = cu_sp.get();
> ----------------
> ki.stfu wrote:
>> Isn't it always false?
> Code elsewhere checks for it, so I assume there are cases when cu_sp can be null.  Example in source/Core/SearchFilter.cpp:
>        for (size_t i = 0; i < num_comp_units; i++)
>        {
>            CompUnitSP cu_sp (module_sp->GetCompileUnitAtIndex (i));
>            if (cu_sp)
>            {
> 
> Better safe than sorry.  
> 
> ================
> Comment at: source/Commands/CommandObjectTarget.cpp:1543
> @@ +1542,3 @@
> +                    continue;
> +                CompileUnit *cu = cu_sp.get();
> +                const FileSpecList &cu_file_list = cu->GetSupportFiles();
> ----------------
> ki.stfu wrote:
>> You don't need a raw pointer here, just use cu_sp.get() on line #1587
> There are 5 uses of cu in this code, so I think it's cleaner to have a variable.
> 
> ================
> Comment at: source/Commands/CommandObjectTarget.cpp:1594-1596
> @@ +1593,5 @@
> +                        // Anymore after this one?
> +                        start_idx++;
> +                        start_idx = cu->FindLineEntry(start_idx, line, &cu_file_spec,
> +                                                      /*exact=*/true, &line_entry);
> +                    } while (start_idx != UINT32_MAX);
> ----------------
> ki.stfu wrote:
>> combine it together:
>> ```
>> cu->FindLineEntry(start_idx + 1, ...)
>> ```
> I'd love to have a guideline as to when to wrap lines - lldb is all over the place about this.  I've tended to try to keep lines to 100.

The lldb coding conventions (http://lldb.llvm.org/lldb-coding-conventions.html) say:

Source code width:

lldb does not follow the 80 character line restriction llvm imposes. In our experience, trying to fit C++ code into an 80 character line results in code that is awkward to read, and the time spent trying to find good indentation points to avoid this would be much better spent on thinking about your code.

More importantly, the restriction induces coders to choose overly abbreviated names to make them better fit in 80 characters. In our opinion choosing good descriptive names is much more important than fitting in 80 characters.

In lldb the limit for code lines is 120 characters because it gets awkward to scan longer lines even on a fairly big monitor, and we've found at that length you seldom have to make code look ugly to get it to wrap.

However you will see some instances of longer lines. The most common occurrence is in the options tables for the CommandInterpreter, which contain the help strings as well as a bunch of important but hard to remember fields. These tables are much easier to read if all the fields line up vertically, and don't have help text interleaved in between the lines. This is another thing to keep in mind when running clang-format, as it will always wrap at 120, so you will need to tweak its output when running against intentionally too-long lines.

Jim

> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D15593
> 
> 
>