Page MenuHomePhabricator

shafik (Shafik Yaghmour)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 12 2018, 2:31 PM (36 w, 2 d)

Recent Activity

Yesterday

shafik added inline comments to D53757: [ASTImporter] Changed use of Import to Import_New in ASTNodeImporter..
Fri, Mar 22, 9:56 AM · Restricted Project

Thu, Mar 21

shafik added a comment to D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName.

I was not able to come up with a test that would detect this issue using either clang-import-test nor via any of the methods used in ASTImpoterTest.cpp. I created a regression test on the lldb side, which should pass once this is committed:

Thu, Mar 21, 2:10 PM
shafik created D59667: Regression test to ensure that we handling importing of anonymous enums correctly.
Thu, Mar 21, 2:09 PM
shafik created D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName.
Thu, Mar 21, 1:59 PM
shafik added a comment to D59433: Fix UUID decoding from minidump files..

It looks like this commit introduced undefined behavior via a misaligned load see this log from lldb sanitized bot on green dragon

Thu, Mar 21, 10:23 AM · Restricted Project

Mon, Mar 18

shafik accepted D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec.

LGTM

Mon, Mar 18, 2:10 PM · Restricted Project, Restricted Project

Fri, Mar 15

shafik accepted D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec.

LGTM

Fri, Mar 15, 9:38 AM · Restricted Project, Restricted Project

Wed, Mar 6

shafik accepted D59040: Move ExpressionSourceCode.cpp -> ClangExpressionSourceCode.cpp.

LGTM although there are some good comments by @aprantl

Wed, Mar 6, 1:02 PM · Restricted Project
shafik committed rG641d0b8cee45: Adding test to cover the correct import of SourceLocation pertaining to a built… (authored by shafik).
Adding test to cover the correct import of SourceLocation pertaining to a built…
Wed, Mar 6, 10:04 AM
shafik committed rL355525: Adding test to cover the correct import of SourceLocation pertaining to a built….
Adding test to cover the correct import of SourceLocation pertaining to a built…
Wed, Mar 6, 10:04 AM
shafik committed rLLDB355525: Adding test to cover the correct import of SourceLocation pertaining to a built….
Adding test to cover the correct import of SourceLocation pertaining to a built…
Wed, Mar 6, 10:04 AM
shafik closed D58790: Adding test to cover the correct import of SourceLocation pertaining to a built-in during expression parsing .
Wed, Mar 6, 10:04 AM · Restricted Project

Tue, Mar 5

shafik accepted D58830: [ASTImporter] Import member expr with explicit template args.

LGTM and I don't see any regressions.

Tue, Mar 5, 9:24 PM · Restricted Project, Restricted Project
shafik added inline comments to D59004: [lldb] Fix DW_OP_addrx uses..
Tue, Mar 5, 3:44 PM · Restricted Project, Restricted Project
shafik added reviewers for D59004: [lldb] Fix DW_OP_addrx uses.: aprantl, jingham, davide.
Tue, Mar 5, 3:41 PM · Restricted Project, Restricted Project
shafik added inline comments to D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec.
Tue, Mar 5, 1:13 PM · Restricted Project, Restricted Project
shafik accepted D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls.

LGTM outside of the question I had.

Tue, Mar 5, 11:07 AM · Restricted Project
shafik committed rGbb322e79c16c: [DataFormatters] Fix regression in libc++ std::atomic formatter caused by https… (authored by shafik).
[DataFormatters] Fix regression in libc++ std::atomic formatter caused by https…
Tue, Mar 5, 10:34 AM
shafik committed rLLDB355422: [DataFormatters] Fix regression in libc++ std::atomic formatter caused by https….
[DataFormatters] Fix regression in libc++ std::atomic formatter caused by https…
Tue, Mar 5, 10:34 AM
shafik committed rL355422: [DataFormatters] Fix regression in libc++ std::atomic formatter caused by https….
[DataFormatters] Fix regression in libc++ std::atomic formatter caused by https…
Tue, Mar 5, 10:33 AM

Mon, Mar 4

shafik committed rG6ed191093d02: Revert "[DataFormatters] Fix regression in libc++ std::atomic formatter caused… (authored by shafik).
Revert "[DataFormatters] Fix regression in libc++ std::atomic formatter caused…
Mon, Mar 4, 4:30 PM
shafik committed rLLDB355352: Revert "[DataFormatters] Fix regression in libc++ std::atomic formatter caused….
Revert "[DataFormatters] Fix regression in libc++ std::atomic formatter caused…
Mon, Mar 4, 4:30 PM
shafik committed rL355352: Revert "[DataFormatters] Fix regression in libc++ std::atomic formatter caused….
Revert "[DataFormatters] Fix regression in libc++ std::atomic formatter caused…
Mon, Mar 4, 4:30 PM
shafik committed rGd38e41ec60cc: [DataFormatters] Fix regression in libc++ std::atomic formatter caused by https… (authored by shafik).
[DataFormatters] Fix regression in libc++ std::atomic formatter caused by https…
Mon, Mar 4, 4:17 PM
shafik committed rL355351: [DataFormatters] Fix regression in libc++ std::atomic formatter caused by https….
[DataFormatters] Fix regression in libc++ std::atomic formatter caused by https…
Mon, Mar 4, 4:16 PM
shafik committed rLLDB355351: [DataFormatters] Fix regression in libc++ std::atomic formatter caused by https….
[DataFormatters] Fix regression in libc++ std::atomic formatter caused by https…
Mon, Mar 4, 4:16 PM
shafik added a comment to D56913: decoupling Freestanding atomic<T> from libatomic.a.

This commit broke the atomic lldb data formatter.

http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/21037/

@shafik @jingham

It looks like LLDB is naming internal names here. Unfortunately, it now needs to learn the new names. On the plus side, there is now a single stable internal name for all compilation paths for this header, whereas it used to differ. So the final result ought to be seen as an improvement.

Nobody disagrees but the formatter has to be fixed, hence the two people cc:ed.

Sorry -- I should have run the tests for the LLDB formatters. This is not the first time it happens.

However, seeing how error-prone it is to change something in libc++ without running the LLDB tests, would it make sense to test the data formatters as part of libc++ itself? One way to see the data formatters is as something that libc++ provides to work nicely with LLDB instead of as something LLDB tries to provide for libc++. When seen that way, it makes more sense that the data formatters use private parts of libc++, and the tests should be part of libc++'s test suite. I don't know how big of a change this is, but I'd encourage LLDB folks to consider this as this would reduce a cause of semi-frequent mistakes.

Mon, Mar 4, 4:09 PM · Restricted Project
shafik committed rG9adbbcb7cd06: [ASTImporter] Handle built-in when importing SourceLocation and FileID (authored by shafik).
[ASTImporter] Handle built-in when importing SourceLocation and FileID
Mon, Mar 4, 12:27 PM
shafik committed rC355332: [ASTImporter] Handle built-in when importing SourceLocation and FileID.
[ASTImporter] Handle built-in when importing SourceLocation and FileID
Mon, Mar 4, 12:26 PM
shafik committed rL355332: [ASTImporter] Handle built-in when importing SourceLocation and FileID.
[ASTImporter] Handle built-in when importing SourceLocation and FileID
Mon, Mar 4, 12:26 PM
shafik closed D58743: Handle built-in when importing SourceLocation and FileID .
Mon, Mar 4, 12:26 PM · Restricted Project
shafik accepted D58502: [ASTImporter] Fix redecl failures of Class and ClassTemplate.

LGTM, thank you !

Mon, Mar 4, 11:53 AM · Restricted Project, Restricted Project

Fri, Mar 1

shafik added a comment to D58792: Add "operator bool" to SB APIs.

It stood out to me that some of the conversions were not const and I can see that IsValid is not consistently const across the API but after talking to @jingham it is unfortunately something we can't change.

Yes, that is unfortunate. I can think of three things that we could do differently though:

  1. add a const version of IsValid where it is missing, and have and always-const operator bool which uses that
  2. give up on constness and just have a non-const operator bool everywhere
  3. add a const operator bool everywhere, and have IsValid (const or non-const) call into that

"const" is useless when your class contains a shared pointer or a unique pointer. It also changes the API mangling which we can't do. So I vote for give up on const as you can call non const methods in any shared and unique pointers because const is only protecting the pointer from changing.

Each option has different tradeoffs, and it's not really clear to me which one is better. I am happy to implement whichever you think is best.

Fri, Mar 1, 2:29 PM · Restricted Project
shafik added a comment to D58743: Handle built-in when importing SourceLocation and FileID .

@teemperor ping

Fri, Mar 1, 9:27 AM · Restricted Project

Thu, Feb 28

shafik added a comment to D58792: Add "operator bool" to SB APIs.

It stood out to me that some of the conversions were not const and I can see that IsValid is not consistently const across the API but after talking to @jingham it is unfortunately something we can't change.

Thu, Feb 28, 1:53 PM · Restricted Project
shafik updated the diff for D58790: Adding test to cover the correct import of SourceLocation pertaining to a built-in during expression parsing .

Addressing comments

Thu, Feb 28, 1:05 PM · Restricted Project
shafik added a comment to D58743: Handle built-in when importing SourceLocation and FileID .

Is there a test missing here?

Thu, Feb 28, 12:25 PM · Restricted Project
shafik created D58790: Adding test to cover the correct import of SourceLocation pertaining to a built-in during expression parsing .
Thu, Feb 28, 12:14 PM · Restricted Project
shafik updated the diff for D58743: Handle built-in when importing SourceLocation and FileID .

Addressed comments on formatting and missing changes to the Import_New version of the code.

Thu, Feb 28, 10:20 AM · Restricted Project

Wed, Feb 27

shafik created D58743: Handle built-in when importing SourceLocation and FileID .
Wed, Feb 27, 3:08 PM · Restricted Project

Tue, Feb 26

shafik accepted D57590: [ASTImporter] Improve import of FileID..

LGTM I don't see any LLDB test regressions but I would prefer another reviewer as well.

Tue, Feb 26, 1:14 PM · Restricted Project

Feb 15 2019

shafik added a comment to D58292: Add support for importing ChooseExpr AST nodes..

This looks reasonable, I will wait for @martong and/or @a_sidorin to review.

Feb 15 2019, 12:12 PM · Restricted Project, Restricted Project
shafik accepted D57910: [ASTImporter] Find previous friend function template.

LGTM, I just ran check-lldb and I don't see any regressions.

Feb 15 2019, 10:49 AM · Restricted Project
shafik added a comment to D58273: Fix TestDataFormatterLibcxxListLoop.py test.

Thanks for fixing this.

Feb 15 2019, 10:35 AM · Restricted Project, Restricted Project

Feb 14 2019

shafik added a comment to D55653: [lldb-mi] Check raw pointers before passing them to std::string ctor/assignment.

Looks like a great fix!

Feb 14 2019, 12:05 PM · Restricted Project

Feb 12 2019

shafik added inline comments to D58125: Add ability to import std module into expression parser to improve C++ debugging.
Feb 12 2019, 9:42 PM · Restricted Project, Restricted Project, Restricted Project
shafik added inline comments to D58090: Deserialize Clang module search path from DWARF.
Feb 12 2019, 2:40 PM · Restricted Project
shafik accepted D57232: [ASTImporter] Check visibility/linkage of functions and variables.

I ran check-lldb locally and it looks good.

Feb 12 2019, 10:03 AM · Restricted Project, Restricted Project

Feb 7 2019

shafik added inline comments to D57902: [AST] Fix structural inequivalence of operators.
Feb 7 2019, 5:26 PM · Restricted Project

Feb 6 2019

shafik accepted D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way.

This looks reasonable to me but I don't have strong opinions on refactoring gtest tests.

Feb 6 2019, 4:37 PM · Restricted Project
shafik accepted D57740: [ASTImporter] Import every Decl in lambda record.
Feb 6 2019, 1:31 PM · Restricted Project, Restricted Project
shafik added a comment to D57740: [ASTImporter] Import every Decl in lambda record.

LGTM although I wish we had more lambda tests.

Feb 6 2019, 1:30 PM · Restricted Project, Restricted Project

Jan 31 2019

shafik committed rL352772: Fix use of non-existing variable in crashlog.py.
Fix use of non-existing variable in crashlog.py
Jan 31 2019, 9:58 AM
shafik committed rLLDB352772: Fix use of non-existing variable in crashlog.py.
Fix use of non-existing variable in crashlog.py
Jan 31 2019, 9:57 AM
shafik closed D57467: Fix use of non-existing variable in crashlog.py.
Jan 31 2019, 9:57 AM

Jan 30 2019

shafik added a comment to D56581: [ASTImporter] Set the described template if not set.

@martong have you had a chance to look at this some more? We ran into another problem that this fixes partially. Can I help in any way?

Jan 30 2019, 3:48 PM
shafik committed rLLDB352677: Fix handling of CreateTemplateParameterList when there is an empty pack.
Fix handling of CreateTemplateParameterList when there is an empty pack
Jan 30 2019, 1:49 PM
shafik committed rL352677: Fix handling of CreateTemplateParameterList when there is an empty pack.
Fix handling of CreateTemplateParameterList when there is an empty pack
Jan 30 2019, 1:49 PM
shafik closed D57363: Fix handling of CreateTemplateParameterList when there is an empty pack.
Jan 30 2019, 1:49 PM
shafik created D57467: Fix use of non-existing variable in crashlog.py.
Jan 30 2019, 12:55 PM

Jan 29 2019

shafik added a comment to D57413: Fix some warnings with gcc on Linux.

To clarify, I think we ought to fix the UB regardless, but Zachary's change can go in anyway as it doesn't make the situation worse than it was before.

Jan 29 2019, 3:09 PM
shafik added inline comments to D57413: Fix some warnings with gcc on Linux.
Jan 29 2019, 2:35 PM
shafik added a comment to D57363: Fix handling of CreateTemplateParameterList when there is an empty pack.

@aprantl @teemperor @davide thank you for the review, some great catches. I believe I have addressed the comments.

Jan 29 2019, 2:22 PM
shafik updated the diff for D57363: Fix handling of CreateTemplateParameterList when there is an empty pack.

Addressed comments, including

  • Renamed test
  • Reduced test size
  • Stream-lined code in CreateTemplateParameterList(...)
Jan 29 2019, 2:22 PM
shafik added inline comments to D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way.
Jan 29 2019, 11:48 AM · Restricted Project

Jan 28 2019

shafik created D57363: Fix handling of CreateTemplateParameterList when there is an empty pack.
Jan 28 2019, 4:26 PM
shafik committed rL352436: [ASTImporter] Fix handling of overriden methods during ASTImport.
[ASTImporter] Fix handling of overriden methods during ASTImport
Jan 28 2019, 1:56 PM
shafik committed rC352436: [ASTImporter] Fix handling of overriden methods during ASTImport.
[ASTImporter] Fix handling of overriden methods during ASTImport
Jan 28 2019, 1:56 PM
shafik closed D56936: Fix handling of overriden methods during ASTImport.
Jan 28 2019, 1:56 PM
shafik updated the diff for D56936: Fix handling of overriden methods during ASTImport.

Addressing comments on commenting and naming.

Jan 28 2019, 11:34 AM

Jan 25 2019

shafik added inline comments to D57232: [ASTImporter] Check visibility/linkage of functions and variables.
Jan 25 2019, 3:10 PM · Restricted Project, Restricted Project
shafik added a comment to D57222: Simplify LangOpts initalization in ClangExpressionParser [NFC].

Nice improvement in readability!

Jan 25 2019, 2:24 PM
shafik updated subscribers of D56581: [ASTImporter] Set the described template if not set.

I don't believe it is directly related to modules but that it just triggers the issue because it may be bringing in more. You can find a description here https://clang.llvm.org/docs/CommandGuide/clang.html#cmdoption-g and @aprantl pointed me to this talk if you want to even more http://llvm.org/devmtg/2015-10/#talk19

Jan 25 2019, 9:41 AM

Jan 24 2019

shafik added a comment to D56581: [ASTImporter] Set the described template if not set.

@martong Unfortunately this causes a regression on one of the lldb tests TestDataFormatterLibcxxVector.py e.g.

Jan 24 2019, 3:11 PM
shafik added a comment to D56936: Fix handling of overriden methods during ASTImport.

@martong so I just tested your lit-tests.patch and it looks good!

Jan 24 2019, 2:45 PM
shafik updated the diff for D56936: Fix handling of overriden methods during ASTImport.

Changes based on comments

  • Refactoring ImportFunctionDeclBody to return Error
  • Refactoring test/Analysis/ctu-main.cpp based on Gabor's patch
  • Removing merging of function body for later PR
Jan 24 2019, 2:44 PM

Jan 23 2019

shafik updated the diff for D56936: Fix handling of overriden methods during ASTImport.

Addressing comments on naming in the unit test and refactoring of duplicated code.

Jan 23 2019, 10:23 PM
shafik added a comment to D56936: Fix handling of overriden methods during ASTImport.

@martong I don't follow the discussion on VisitParmVarDecl an what seems like an alternate fix. Can you elaborate?

Jan 23 2019, 10:22 PM

Jan 18 2019

shafik added a comment to D56936: Fix handling of overriden methods during ASTImport.

@martong the only issue is that I am seeing a regression on Analysis/ctu-main.cpp when I run check-clang. I am going to look into it but if you have any insights that would be helpful.

Jan 18 2019, 2:11 PM
shafik created D56936: Fix handling of overriden methods during ASTImport.
Jan 18 2019, 2:07 PM

Jan 16 2019

shafik added a comment to D56651: [ASTImporter] Fix importing OperatorDelete from CXXConstructorDecl.

Thank you, this looks good but perhaps some refactoring would help improve the change.

Jan 16 2019, 9:58 AM

Jan 15 2019

shafik added a comment to D56688: Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC).

Sorry for the late review

Jan 15 2019, 12:16 PM

Jan 11 2019

shafik added a comment to D56581: [ASTImporter] Set the described template if not set.

Can you add a test?

Jan 11 2019, 10:06 AM
shafik accepted D53699: [ASTImporter] Fix inequality of functions with different attributes.

Thank you for adding the additional test.

Jan 11 2019, 10:01 AM

Dec 12 2018

shafik requested changes to D53699: [ASTImporter] Fix inequality of functions with different attributes.
Dec 12 2018, 11:27 AM
shafik added inline comments to D53699: [ASTImporter] Fix inequality of functions with different attributes.
Dec 12 2018, 11:16 AM
shafik added a comment to D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull.

Sounds good!

Dec 12 2018, 10:52 AM
shafik accepted D55584: [LLDB] Simplify Boolean expressions.

LGTM after comment are addressed.

Dec 12 2018, 10:03 AM · Restricted Project

Dec 11 2018

shafik added a comment to D55584: [LLDB] Simplify Boolean expressions.

I find the static_cast<bool> to be a bit too clever, I don't think it helps readability.

Dec 11 2018, 3:21 PM · Restricted Project

Dec 10 2018

shafik added a comment to D55045: Add a version of std::function that includes a few optimizations..

See changes here

Dec 10 2018, 4:13 PM
shafik added a comment to D55045: Add a version of std::function that includes a few optimizations..

The fix was not too bad, I landed the fix and the bots just turned green http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/14015/

Dec 10 2018, 3:56 PM
shafik committed rLLDB348810: [DataFormatters] Fixes to libc++ std::function formatter to deal with ABI….
[DataFormatters] Fixes to libc++ std::function formatter to deal with ABI…
Dec 10 2018, 3:32 PM
shafik committed rL348810: [DataFormatters] Fixes to libc++ std::function formatter to deal with ABI….
[DataFormatters] Fixes to libc++ std::function formatter to deal with ABI…
Dec 10 2018, 3:32 PM
shafik added a comment to D55045: Add a version of std::function that includes a few optimizations..

Just a heads up this change broke the lldb std::function formatter see the logs here:

Dec 10 2018, 12:42 PM

Dec 7 2018

shafik committed rLLDB348629: Revert "Introduce ObjectFileBreakpad".
Revert "Introduce ObjectFileBreakpad"
Dec 7 2018, 11:02 AM
shafik committed rL348629: Revert "Introduce ObjectFileBreakpad".
Revert "Introduce ObjectFileBreakpad"
Dec 7 2018, 11:02 AM

Dec 6 2018

shafik updated subscribers of D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules().

This PR broke the green dragon bots, see the following log file:

Dec 6 2018, 3:48 PM

Nov 28 2018

shafik added a reviewer for D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull: teemperor.
Nov 28 2018, 11:29 AM
shafik added a comment to D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull.

As pointed out in this comment in another review

Nov 28 2018, 11:28 AM
shafik added a comment to D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger.

Apologies, I meant to make the comment in the child PR

Nov 28 2018, 11:24 AM · Restricted Project