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

[Dawn Perchik via lldb-commits]

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.

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.


Repository:
  rL LLVM

http://reviews.llvm.org/D15593