[Lldb-commits] [PATCH] D16769: [NFC] Cleanup RangeMap.h

[Tamas Berghammer via lldb-commits]

tberghammer added a comment.

In http://reviews.llvm.org/D16769#340936, @clayborg wrote:

> Looks fine. A few things I don't like, but I can live with:
>
> - Making constructors appear all on one line does save space, but it means you can't debug very well as stepping into these constructors will stop on the constructor line and then if you step, you will step  over and out of the entire thing. Granted these classes are very simple and probably don't require debugging anymore, but it is a drawback to clang-format.


I think if this is not the format we want then we should check clang-format to support our use case. In this specific case I think putting everything in 1 line is fine as you see the function arguments when you step in and I also think clang-format will do something different for constructors with more code in them.

> - Many functions had variables that were put into temporaries like:

> 

>   ``` bool DoesAdjoinOrIntersect (const Range &rhs) const { const BaseType lhs_base = this->GetRangeBase(); const BaseType rhs_base = rhs.GetRangeBase(); const BaseType lhs_end = this->GetRangeEnd(); const BaseType rhs_end = rhs.GetRangeEnd(); bool result = (lhs_base <= rhs_end) && (lhs_end >= rhs_base); return result; } ```

> 

>   were reduced to lines like the following:

> 

>   ``` bool DoesAdjoinOrIntersect(const Range &rhs) const { return GetRangeBase() <= rhs.GetRangeEnd() && GetRangeEnd() >= rhs.GetRangeBase(); } ``` So I see the efficiency, but if I step into this function now it means less useful debugging as you will be stepping into and out of functions and you will lose the values and not be able to see what is going on. The compiler will get rid of the temporary variables when it optimizes things so I am not sure why these changes make things better.


I made these changes intentionally because I feel that inlining these temporaries improve the readability as you don't have to match the very similar 6-7 letter words by eye and it shouldn't hurt debugging too much because you can either evaluate the expression you are interested or just display "*this" or "rhs" as a frame variable. From efficiency perspective they won't matter if the types used are basic types (PODs) but if the compiler can optimize them out if they have a non-trivial constructor/destructor. I can revert those if you want.


http://reviews.llvm.org/D16769