Thank you a lot for explanations and commit!
On Thu, Jul 19, 2018 at 4:40 PM Pavel Labath wrote:
> Thanks.
>
> I agree you're not in the best position to make these kinds of changes
> (and I don't think I would have asked you to do them). In fact, I was
> already considering just changing that
Thanks.
I agree you're not in the best position to make these kinds of changes
(and I don't think I would have asked you to do them). In fact, I was
already considering just changing that function myself, so I went
ahead and did that now in r337452. As far as I can tell, no tests need
to be
On Thu, Jul 19, 2018 at 2:14 PM Pavel Labath wrote:
> I knew I should have stayed quiet :P, but now that I am in, here's my
> reasoning:
>
Thank you for not staying quiet, I think it's the only way to have a dialog
and solve the problems :)
I find your argumentation convincing. The problem is
I knew I should have stayed quiet :P, but now that I am in, here's my reasoning:
> - For eight years (the spaces were commited in 2010) some tests may have
> relied on these spaces. It's a good case - we can run tests and see it;
For seven of those eight years we haven't been using this function
But can you, please, explain me why?
Excuse me for so much fuss, I just wanted to explain my position... We have
two cases now:
- Just commit such a patch without an additional investigation. But it's
not a good idea, because we can't be 100% sure that it is a typo (and even
if it is, we still
On Thu, 19 Jul 2018 at 10:04, Aleksandr Urakov via Phabricator
wrote:
> So peoples who commited such spaces found them beautiful (or used the
> approved formatting style, or used them for some special formatting cases),
Or it's simply a typo.
I don't want to make a bigger fuss of this than it
aleksandr.urakov added a comment.
Can you explain me, please, why do you think that we should remove these spaces?
I have the following considerations:
- For eight years (the spaces were commited in 2010) some tests may have relied
on these spaces. It's a good case - we can run tests and see
stella.stamenova added a comment.
Can you test this change and send it for review? Thanks!
Repository:
rL LLVM
https://reviews.llvm.org/D49018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
aleksandr.urakov added a comment.
I've removed spaces in such places, but i'm not sure if this won't break
anything (may be something relies on that spaces). @clayborg, are these spaces
have some special purpose? I think it's a decor and not really
important.F6716378: patch.diff
stella.stamenova added a comment.
Please fix the space. The test will process it fine, but that doesn't mean we
shouldn't fix it. I'll email you with the setup we use for tests.
Repository:
rL LLVM
https://reviews.llvm.org/D49018
___
aleksandr.urakov added a comment.
The expression `location = {{.*}},` should process that space correctly. It is
possible to remove that space in `DWARFExpression::DumpLocation` function, but
I don't know how necessary is it.
But what about a compiler version? I want to fix the issue if
stella.stamenova added a comment.
When you send a review for the fix, please add me to it. Could you also take
care of the extra space in the output? Thanks!
Repository:
rL LLVM
https://reviews.llvm.org/D49018
___
lldb-commits mailing list
aleksandr.urakov added a comment.
Yes, you are right, extra `location = DW_OP_addr(000140004114)` appeared
after this commit. But I hadn't noticed that, because the test was already
broken for me on `0fa537f42f1af238c74bf41998dc1af31195839a`. Now I have checked
one out and ran the test to
stella.stamenova added a comment.
The CHECK-SAME expression on line 10 can no longer find the expected string in
the output. This is due to an extra `location = DW_OP_addr(000140004114) ,`
in the output between the two expected strings `CHECK-SAME: scope = global,
external`, so it looks
zturner added a comment.
Fwiw I’ve seen cases where tests have passed even though they shouldn’t
have — the functionality being tested was broken. The one that comes to
mind was where we were doing a backtrace and then checking that it matched
the regex “main\(argc=3” to make sure the local
Fwiw I’ve seen cases where tests have passed even though they shouldn’t
have — the functionality being tested was broken. The one that comes to
mind was where we were doing a backtrace and then checking that it matched
the regex “main\(argc=3” to make sure the local variable argc had the
correct
stella.stamenova added a comment.
I'll spend some time looking into this today, but with commit
0fa537f42f1af238c74bf41998dc1af31195839a variables.test passes. Then with
commit d9899ad86e0a9b05781015cacced1438fcf70343, the test fails. There are
clearly a couple of other commits in that range,
aleksandr.urakov added a comment.
This doesn't look like the cause, the test fails for me without this patch...
Here is my tests output for PDB folder:
-- Testing: 10 tests, 8 threads --
FAIL: lldb :: SymbolFile/PDB/enums-layout.test (1 of 10)
FAIL: lldb :: SymbolFile/PDB/typedefs.test (2
stella.stamenova added a comment.
I am not 100% sure that this is the cause yet, but the test variables.test is
now failing on Windows. @aleksandr.urakov and @JDevlieghere, which tests did
you run on Windows? Did they all pass?
Repository:
rL LLVM
https://reviews.llvm.org/D49018
aleksandr.urakov added a comment.
Thank you!
Repository:
rL LLVM
https://reviews.llvm.org/D49018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336988: Convert a location information from PDB to a DWARF
expression (authored by JDevlieghere, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
asmith added a subscriber: aleksandr.urakov.
asmith added a comment.
Sure, I will do it Monday if someone hasn’t done it already.
https://reviews.llvm.org/D49018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
Sure, I will do it Monday if someone hasn’t done it already.
> On Jul 12, 2018, at 9:22 PM, Aleksandr Urakov via Phabricator
> wrote:
>
> aleksandr.urakov updated this revision to Diff 155154.
> aleksandr.urakov added a comment.
>
> Thanks a lot, I have updated the patch!
>
> I also have
asmith accepted this revision.
asmith added a comment.
This revision is now accepted and ready to land.
Except for minor formatting LGTM
Comment at: lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.cpp:7
+
+ return;
+}
Please remove the return
aleksandr.urakov added a comment.
Ping!
Can you review this, please? I have just implemented the UDT types completion
for PDB symbol files and I am preparing a patch for that, but it makes no sense
without the part implemented in this one :)
https://reviews.llvm.org/D49018
aleksandr.urakov added a comment.
Excuse me, I have forgot to add lldb-commits as a subscriber, so I'll repeat
initial message.
The current version of SymbolFilePDB::ParseVariableForPDBData function always
initializes variables with an empty location. This patch adds the converter of
a
aleksandr.urakov added a comment.
The test here can be expanded to include https://reviews.llvm.org/D48960 case.
https://reviews.llvm.org/D49018
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
27 matches
Mail list logo
