vsk created this revision.
vsk added reviewers: labath, zturner, davide, aprantl, lhames.
LLDB CompilerTypes may be invalid, i.e they should be checked for
validity before use.
Compared to a more typical model in which only valid types exist [1],
this has a few disadvantages:
- The debugger is g
aprantl added a comment.
> Going forward, we should transition to a model in which CompilerTypes are
> either valid or do not exist.
I don't understand very well how the LLDB type system works so excuse my naive
questions: Does this account for lazyness? I.e., could there be value in having
an
JDevlieghere added inline comments.
Comment at: include/lldb/Symbol/CompilerType.h:446
+/// A force-checked error used to describe type construction failures.
+class InvalidType : public llvm::ErrorInfo {
+public:
I think this should be called `InvalidTypeError`?
labath added a comment.
I agree that we should start using this in a lot more places. I think you
should be able to shorten the logging code a bit by using the LLDB_LOG_ERROR
macro, which I've created for this purpose.
Comment at:
source/Plugins/ExpressionParser/Clang/ClangE
zturner added inline comments.
Comment at:
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:382-387
+ if (llvm::Error E = user_type_or_err.takeError()) {
+std::string Reason = llvm::toString(std::move(E));
if (log)
- log->Printf("Persistent variabl
jingham added a comment.
In https://reviews.llvm.org/D43912#1022916, @aprantl wrote:
> > Going forward, we should transition to a model in which CompilerTypes are
> > either valid or do not exist.
>
> I don't understand very well how the LLDB type system works so excuse my
> naive questions: Do
vsk added inline comments.
Comment at:
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:335-340
+if (llvm::Error E = user_type_or_err.takeError()) {
+ std::string Reason = llvm::toString(std::move(E));
+ if (log)
+log->Printf("%s", Reason.c_
vsk updated this revision to Diff 136433.
vsk marked an inline comment as done.
vsk edited the summary of this revision.
vsk added a comment.
- Address some review feedback.
https://reviews.llvm.org/D43912
Files:
include/lldb/Symbol/CompilerType.h
include/lldb/Utility/Log.h
source/Plugins
vsk added a comment.
In https://reviews.llvm.org/D43912#1023078, @jingham wrote:
> In https://reviews.llvm.org/D43912#1022916, @aprantl wrote:
>
> > > Going forward, we should transition to a model in which CompilerTypes are
> > > either valid or do not exist.
> >
> > I don't understand very wel
jingham added a comment.
The problem with your RETURN_IF_UNEXPECTED macro is that it make it impossible
to put a breakpoint only on the "type was invalid case." To catch that
happening while debugging you'd have to write a conditional breakpoint that
also checks the error.
That seems like som
labath added inline comments.
Comment at: include/lldb/Utility/Log.h:249-254
+ ::lldb_private::Log *log_private = (log);
\
+ if (log_private)
\
+log_private->FormatError(::s
jingham added a comment.
The phase where we are converting everything over seems exactly when we want to
be able to easily stop on the error cases when debugging, which using a macro
that contains the return makes impossible.
https://reviews.llvm.org/D43912
_
vsk updated this revision to Diff 136450.
vsk added a comment.
> The problem with your RETURN_IF_UNEXPECTED macro is that it make it
> impossible to put a breakpoint only on the "type was invalid case." To catch
> that happening while debugging you'd have to write a conditional breakpoint
> tha
vsk added inline comments.
Comment at: include/lldb/Utility/Log.h:249-254
+ ::lldb_private::Log *log_private = (log);
\
+ if (log_private)
\
+log_private->FormatError(::std:
jingham added a comment.
Asking the person trying to catch a bad CompilerType to understand how the
error mechanism works to effect this goal seems a little much, especially
'cause they are going to be focused on the return which your macro would make
tantalizingly out of my reach. OTOH, if yo
zturner added a comment.
I'm also ok with not having the macro fwiw, just an idea to reduce boilerplate.
https://reviews.llvm.org/D43912
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-co
What about having the macro do the log printing in a function so you can
put the breakpoint in the function?
On Wed, Feb 28, 2018 at 5:56 PM Vedant Kumar via Phabricator <
revi...@reviews.llvm.org> wrote:
> vsk added inline comments.
>
>
>
> Comment at: include/lldb/Utility/Log.h:
I have no general objections to macros, and reducing boiler-plate is good.
They do get in the way of debugging because of the weird C rule that a macro
has to pretend that it is all one source line, so if they contain code you are
interested in stopping at, there needs to be some other way to d
> On Mar 1, 2018, at 10:25 AM, Jim Ingham wrote:
>
> I have no general objections to macros, and reducing boiler-plate is good.
> They do get in the way of debugging because of the weird C rule that a macro
> has to pretend that it is all one source line, so if they contain code you
> are in
On 1 March 2018 at 10:34, Adrian Prantl wrote:
>
>> On Mar 1, 2018, at 10:25 AM, Jim Ingham wrote:
>>
>> I have no general objections to macros, and reducing boiler-plate is good.
>> They do get in the way of debugging because of the weird C rule that a macro
>> has to pretend that it is all o
> On Mar 1, 2018, at 10:44 AM, Pavel Labath wrote:
>
> On 1 March 2018 at 10:34, Adrian Prantl wrote:
>>
>>> On Mar 1, 2018, at 10:25 AM, Jim Ingham wrote:
>>>
>>> I have no general objections to macros, and reducing boiler-plate is good.
>>> They do get in the way of debugging because of
21 matches
Mail list logo
