Page MenuHomePhabricator

shafik (Shafik Yaghmour)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Fri, Jan 18

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.

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

Wed, Jan 16

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.

Wed, Jan 16, 9:58 AM

Tue, Jan 15

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

Sorry for the late review

Tue, Jan 15, 12:16 PM

Fri, Jan 11

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

Can you add a test?

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

Thank you for adding the additional test.

Fri, Jan 11, 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
shafik added a comment to D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger.

As pointed out in this comment in another review

Nov 28 2018, 11:00 AM · Restricted Project
shafik added a comment to D53751: [ASTImporter] Added Import functions for transition to new API..

I didn't actually see this comment get addressed other than to say it won't be a problem in practice, which I'm not certain I agree with. Was there a reason why this got commit before finding out if the reviewer with the concern agrees with your rationale? FWIW, I share the concern that having parallel APIs for any length of time is a dangerous thing. Given that "almost ready to go" is not "ready to go" but there's not an imminent release, I don't understand the rush to commit this.

@aaron.ballman Thanks for your time and review on this.

I completely understand you concern and agree that having such parallel API even for a short time is not good. Please let me explain why we chose to do this still:
ASTImporter::Import functions are used externally by LLDB and CTU as clients. However, the functions are used internally too, inside ASTImporter and ASTNodeImporter. E.g. when we import an expression, then first we use the Import(QualType) function to import its type.
Our goal is twofold: (1) enforce ASTImporter and ASTNodeImporter implementation functions to always check the result of used import functions and (2) make sure that clients can have detailed error information, so e.g. in case of CTU we can handle unsupported constructs differently than ODR errors.
As @balazske mentioned we could have changed the API in one lockstep but the impact would have been too huge.

Nov 28 2018, 10:49 AM
shafik added a comment to D53693: [ASTImporter] Typedef import brings in the complete type.

Apologies for my late comments.

Nov 28 2018, 10:34 AM

Nov 27 2018

shafik requested changes to D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull.

The rest of the changes look good.

Nov 27 2018, 2:46 PM

Nov 16 2018

shafik added a reviewer for D53751: [ASTImporter] Added Import functions for transition to new API.: rsmith.
Nov 16 2018, 1:01 PM
Herald added a reviewer for D53751: [ASTImporter] Added Import functions for transition to new API.: shafik.

I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.

Nov 16 2018, 12:58 PM

Nov 15 2018

shafik added a comment to D53702: [ASTImporter] Set redecl chain of functions before any other import.

Did you ever resolve the issue of libcxx tests not running https://reviews.llvm.org/D53697

Nov 15 2018, 10:28 AM
shafik added inline comments to D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible.
Nov 15 2018, 10:14 AM · Restricted Project

Nov 14 2018

shafik accepted D54528: [lldb] NFC: Remove the extra ';'.

LGTM, thank you for fixing this.

Nov 14 2018, 9:29 AM

Nov 13 2018

shafik added inline comments to D54452: [NativePDB] Add support for handling S_CONSTANT records.
Nov 13 2018, 9:48 AM

Nov 12 2018

shafik added a comment to D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

@zturner I would not be against discussing using pass by value for small objects going forward. I don't currently have a good feeling for at what sizes/data types the right trade-off is at though.

Nov 12 2018, 4:33 PM
shafik added inline comments to D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible.
Nov 12 2018, 12:50 PM · Restricted Project

Nov 8 2018

shafik committed rLLDB346428: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove….
Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove…
Nov 8 2018, 10:45 AM
shafik committed rL346428: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove….
Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove…
Nov 8 2018, 10:44 AM
shafik closed D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.
Nov 8 2018, 10:44 AM

Nov 7 2018

shafik added inline comments to D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.
Nov 7 2018, 4:34 PM
shafik added a comment to D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

@jingham @zturner @teemperor @clayborg I believe I have addressed all the comments.

Nov 7 2018, 4:34 PM
shafik updated the diff for D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

Made changes based on comments.

Nov 7 2018, 4:34 PM
shafik added a comment to D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

@zturner I was out of date when I posted this diff but I have that file updated locally and it will show up when I update the diff.

Nov 7 2018, 2:32 PM
shafik added a comment to D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

@zturner I don't see AddEnumerationValueToEnumerationType being called in SymbolFile/NativePDB.cpp https://github.com/llvm-mirror/lldb/search?q=AddEnumerationValueToEnumerationType&unscoped_q=AddEnumerationValueToEnumerationType

Nov 7 2018, 2:19 PM

Nov 6 2018

shafik added a comment to D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution.

Much better, thank you!

Nov 6 2018, 10:48 AM

Nov 5 2018

shafik added inline comments to D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution.
Nov 5 2018, 2:13 PM

Nov 1 2018

shafik created D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.
Nov 1 2018, 2:53 PM
shafik added a comment to D53697: [ASTImporter][Structural Eq] Check for isBeingDefined.

@martong Hello, if I look at the specific test that was failing TestDataFormatterLibcxxVector.py it only has the following decorator @add_test_categories(["libc++"]) so that should not prevent it from running on Linux if you are also building libc++. If you do a local cmake build and you build libc++ as well does it still skip the test?

Nov 1 2018, 10:20 AM

Oct 31 2018

shafik added a comment to D53697: [ASTImporter][Structural Eq] Check for isBeingDefined.

I reverted this commit since it caused a regression in the lldb test suite, specifically TestDataFormatterLibcxxVector.py.

Oct 31 2018, 3:00 PM
shafik committed rL345784: Revert "[ASTImporter][Structural Eq] Check for isBeingDefined".
Revert "[ASTImporter][Structural Eq] Check for isBeingDefined"
Oct 31 2018, 2:56 PM
shafik committed rC345784: Revert "[ASTImporter][Structural Eq] Check for isBeingDefined".
Revert "[ASTImporter][Structural Eq] Check for isBeingDefined"
Oct 31 2018, 2:55 PM
shafik added a comment to D44100: [ASTImporter] Reorder fields after structure import is finished.

I am happy to discuss and review a revised version of this change.

Oct 31 2018, 10:13 AM

Oct 29 2018

shafik added a comment to D53788: [FileSystem] Remove GetByteSize() from FileSpec.

Are we refactoring in the right place? Why not just refactor FileSpec::GetByteSize() why is FileSpec the wrong place?

Oct 29 2018, 10:58 AM · Restricted Project

Oct 26 2018

shafik committed rLLDB345402: [DataFormatters] Adding formatters for libc++ std::u16string and std::u32string.
[DataFormatters] Adding formatters for libc++ std::u16string and std::u32string
Oct 26 2018, 10:04 AM
shafik committed rL345402: [DataFormatters] Adding formatters for libc++ std::u16string and std::u32string.
[DataFormatters] Adding formatters for libc++ std::u16string and std::u32string
Oct 26 2018, 10:04 AM
shafik closed D53656: Adding formatters for libc++ std::u16string and std::u32string.
Oct 26 2018, 10:04 AM

Oct 25 2018

shafik accepted D53709: Get rid of C-style cast..
Oct 25 2018, 9:12 AM
shafik added a comment to D53709: Get rid of C-style cast..

LGTM

Oct 25 2018, 9:11 AM
shafik added inline comments to D53677: Fix a bug PlatformDarwin::SDKSupportsModule.
Oct 25 2018, 8:56 AM

Oct 24 2018

shafik created D53656: Adding formatters for libc++ std::u16string and std::u32string.
Oct 24 2018, 9:43 AM

Oct 23 2018

shafik added a comment to D53590: Refactor SetBaseClassesForType and DeleteBaseClasses to be more C++'y.

+1 for modernizing code!

Oct 23 2018, 12:39 PM

Oct 12 2018

shafik added inline comments to D53208: [lldbsuite] Fix the mac version decorator to work on non-mac platforms.
Oct 12 2018, 2:04 PM
shafik added a comment to D49271: Adding libc++ formattors for std::optional.

@stella.stamenova Thank you for catching this. I fixed the test names, I am looking into the best way to fix the skipif now.

Oct 12 2018, 12:54 PM
shafik committed rL344407: Changing test names in TestDataFormatterLibcxxVariant.py and….
Changing test names in TestDataFormatterLibcxxVariant.py and…
Oct 12 2018, 12:48 PM
shafik committed rLLDB344407: Changing test names in TestDataFormatterLibcxxVariant.py and….
Changing test names in TestDataFormatterLibcxxVariant.py and…
Oct 12 2018, 12:48 PM
shafik committed rLLDB344371: Adding support to step into the callable wrapped by libc++ std::function.
Adding support to step into the callable wrapped by libc++ std::function
Oct 12 2018, 10:23 AM
shafik committed rL344371: Adding support to step into the callable wrapped by libc++ std::function.
Adding support to step into the callable wrapped by libc++ std::function
Oct 12 2018, 10:23 AM
shafik closed D52851: Adding support to step into the callable wrapped by libc++ std::function.
Oct 12 2018, 10:23 AM

Oct 11 2018

shafik added a comment to D52851: Adding support to step into the callable wrapped by libc++ std::function.

@jingham removed code and made it NO_DEBUG_INFO_TESTCASE

Oct 11 2018, 11:16 AM
shafik updated the diff for D52851: Adding support to step into the callable wrapped by libc++ std::function.

Removing uneeded code and making test NO_DEBUG_INFO_TESTCASE

Oct 11 2018, 11:16 AM

Oct 10 2018

shafik added a comment to D52851: Adding support to step into the callable wrapped by libc++ std::function.

@jingham I believe I addressed your comments, please review again.

Oct 10 2018, 3:03 PM
shafik updated the diff for D52851: Adding support to step into the callable wrapped by libc++ std::function.
  • Addressing comments
  • Adding test to make sure we step through a std::function wrapping a member variable
Oct 10 2018, 3:03 PM

Oct 4 2018

shafik updated the diff for D52851: Adding support to step into the callable wrapped by libc++ std::function.

Addressing comments and fixing a bug in ThreadPlanStepThrough.cpp where I would overwrite the result of objc_runtime.

Oct 4 2018, 3:08 PM

Oct 3 2018

shafik created D52851: Adding support to step into the callable wrapped by libc++ std::function.
Oct 3 2018, 4:13 PM
shafik committed rLLDB343718: Adding skipIf to std::variant libc++ data-formatter test since get is not….
Adding skipIf to std::variant libc++ data-formatter test since get is not…
Oct 3 2018, 1:55 PM
shafik committed rL343718: Adding skipIf to std::variant libc++ data-formatter test since get is not….
Adding skipIf to std::variant libc++ data-formatter test since get is not…
Oct 3 2018, 1:55 PM

Oct 2 2018

shafik added inline comments to D52788: Add EchoCommentCommands to CommandInterpreterRunOptions in addition to the existing EchoCommands and expose both as interpreter settings..
Oct 2 2018, 11:57 AM
shafik added inline comments to D52788: Add EchoCommentCommands to CommandInterpreterRunOptions in addition to the existing EchoCommands and expose both as interpreter settings..
Oct 2 2018, 11:22 AM

Sep 21 2018

shafik added inline comments to D50254: [RFC] Add GDB remote packet reproducer..
Sep 21 2018, 10:18 AM

Sep 20 2018

shafik added a comment to D50254: [RFC] Add GDB remote packet reproducer..

First batch of comments, may come back to it today.

Sep 20 2018, 2:19 PM
shafik committed rLLDB342663: Refactor FindVariable() core functionality into StackFrame out of SBFrame.
Refactor FindVariable() core functionality into StackFrame out of SBFrame
Sep 20 2018, 10:09 AM
shafik committed rL342663: Refactor FindVariable() core functionality into StackFrame out of SBFrame.
Refactor FindVariable() core functionality into StackFrame out of SBFrame
Sep 20 2018, 10:09 AM
shafik closed D52247: Refactor FindVariable() core functionality into StackFrame out of SBFrame.
Sep 20 2018, 10:09 AM
shafik updated the diff for D52247: Refactor FindVariable() core functionality into StackFrame out of SBFrame.

Adjusting documentation based on feedback

Sep 20 2018, 9:05 AM

Sep 19 2018

shafik added a comment to D52247: Refactor FindVariable() core functionality into StackFrame out of SBFrame.

@jingham @clayborg @davide addressed comments w/ the exception of the lambda one which I politely disagree with.

Sep 19 2018, 3:58 PM
shafik updated the diff for D52247: Refactor FindVariable() core functionality into StackFrame out of SBFrame.

Addressing comments:

Sep 19 2018, 3:54 PM
shafik committed rLLDB342563: [DataFormatters] Add formatter for C++17 std::variant.
[DataFormatters] Add formatter for C++17 std::variant
Sep 19 2018, 11:09 AM
shafik committed rL342563: [DataFormatters] Add formatter for C++17 std::variant.
[DataFormatters] Add formatter for C++17 std::variant
Sep 19 2018, 11:08 AM
shafik updated the diff for D51520: Add libc++ data formatter for std::variant.

Updating LibcxxVariantGetIndexValidity() to no longer do type check of __index. It was left over from the old method of checking for an empty variant and was also breaking clang 5.

Sep 19 2018, 10:26 AM

Sep 18 2018

shafik created D52247: Refactor FindVariable() core functionality into StackFrame out of SBFrame.
Sep 18 2018, 2:01 PM

Sep 17 2018

shafik committed rL342424: Revert "[DataFormatters] Add formatter for C++17 std::variant".
Revert "[DataFormatters] Add formatter for C++17 std::variant"
Sep 17 2018, 4:19 PM
shafik committed rLLDB342424: Revert "[DataFormatters] Add formatter for C++17 std::variant".
Revert "[DataFormatters] Add formatter for C++17 std::variant"
Sep 17 2018, 4:19 PM
shafik committed rLLDB342421: [DataFormatters] Add formatter for C++17 std::variant.
[DataFormatters] Add formatter for C++17 std::variant
Sep 17 2018, 3:12 PM
shafik committed rL342421: [DataFormatters] Add formatter for C++17 std::variant.
[DataFormatters] Add formatter for C++17 std::variant
Sep 17 2018, 3:12 PM
shafik closed D51520: Add libc++ data formatter for std::variant.
Sep 17 2018, 3:12 PM

Sep 14 2018

shafik added a comment to D51520: Add libc++ data formatter for std::variant.

@teemperor Thanks for the catch!

Sep 14 2018, 10:59 AM
shafik updated the diff for D51520: Add libc++ data formatter for std::variant.

Applying clang-format to files I missed earlier

Sep 14 2018, 10:59 AM
shafik updated the diff for D51520: Add libc++ data formatter for std::variant.

Applying clang-format

Sep 14 2018, 10:44 AM
shafik added inline comments to D51934: [target] Change target create's behavior wrt loading dependent files..
Sep 14 2018, 10:12 AM

Sep 13 2018

shafik added a comment to D51520: Add libc++ data formatter for std::variant.

@vsk Interesting I ran git clang-format before generating the diff and it made changes, so I am not sure what happened

Sep 13 2018, 11:35 AM

Sep 12 2018

shafik added a comment to D51520: Add libc++ data formatter for std::variant.

@vsk @jingham I believe I have addressed your comments, please review again.

Sep 12 2018, 9:36 PM
shafik updated the diff for D51520: Add libc++ data formatter for std::variant.

Address comments on std::variant formatter

  • Adding tests for reference to variant and variant of variant
  • Refactoring test to be more efficient
  • Refactoring unsafe code that assumed sizes of types between local machine and target machine were the same
    • Used to check to variant_npos which is max unsigned or -1
Sep 12 2018, 9:35 PM