> On Dec 17, 2015, at 10:25 AM, Dawn Perchik <dawn+l...@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
>
>
>
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
