arphaman (Alex Lorenz)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 25 2014, 4:17 PM (229 w, 4 d)

Recent Activity

Fri, Nov 16

arphaman added a comment to D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one.

Thanks for working on this, some minor comments:

Fri, Nov 16, 10:42 AM
arphaman updated subscribers of D54630: Move detection of libc++ include dirs to Driver on MacOS.
Fri, Nov 16, 10:29 AM
arphaman added a comment to D54630: Move detection of libc++ include dirs to Driver on MacOS.

I don't think we want to move the logic to add a libc++ path to the driver. -cc1 with -resource-dir and -stdlib=libc++ should still work as before. In this case the previous patch is better, except it should not set InstalledDir except when needed (e.g. for tooling when working with a CDB that has an absolute path to the compiler), so when InstalledDir is empty it should fallback to the current logic in InitHeaderSearch.cpp. That should solve the issues we had.

Fri, Nov 16, 10:27 AM
arphaman added a comment to D54547: PTH-- Remove feature entirely-.

Would it be better to deprecate the use of PTH right now for the current release, so that the users will be aware that we will remove PTH support in LLVM 9 once they upgrade to LLVM 8? Then we can remove it right after LLVM 8 branch is created.

Fri, Nov 16, 10:11 AM

Wed, Nov 14

arphaman added a comment to D54529: [clangd] Add USR to textDocument/definition response.

I don't think we should be using textDocument/definition here, and I agree with Sam that a new method would be better. We don't actually need the semantic guarantees/constrains imposed by LSP's description of textDocument/definition, as we want to find any declaration that corresponds to the reference at the location, so we don't need to guarantee that we return some definition. This would also avoid any confusion about USRs returned in textDocument/definition/textDocument/references.

Wed, Nov 14, 5:30 PM · Restricted Project

Tue, Nov 13

arphaman committed rC346822: [HeaderSearch] loadSubdirectoryModuleMaps should respect -working-directory.
[HeaderSearch] loadSubdirectoryModuleMaps should respect -working-directory
Tue, Nov 13, 5:10 PM
arphaman committed rL346822: [HeaderSearch] loadSubdirectoryModuleMaps should respect -working-directory.
[HeaderSearch] loadSubdirectoryModuleMaps should respect -working-directory
Tue, Nov 13, 5:10 PM
arphaman closed D54503: [HeaderSearch] loadSubdirectoryModuleMaps should respect -working-directory.
Tue, Nov 13, 5:10 PM
arphaman created D54503: [HeaderSearch] loadSubdirectoryModuleMaps should respect -working-directory.
Tue, Nov 13, 4:28 PM
arphaman added a comment to D54455: [vfs] add 'Status::copyWithNewSize'.

Sorry for joining in the middle of the conversation without the full context, but what is the use-case for status that were created with the wrong size that needs to be patched up?
It's clear what the function is doing operationally, but it's hard to see why would we want to change the size of the file, while leaving other things (like modification time, etc.) the same?

Tue, Nov 13, 11:11 AM
arphaman abandoned D54455: [vfs] add 'Status::copyWithNewSize'.

The change itself is fine but I wonder if we really need these helper methods and can't just use the copy constructor?

Tue, Nov 13, 11:07 AM

Mon, Nov 12

arphaman created D54455: [vfs] add 'Status::copyWithNewSize'.
Mon, Nov 12, 3:47 PM
arphaman added a comment to D54310: Make clang-based tools find libc++ on MacOS.

Apologies for not seeing this earlier.
I agree with George, this behavior doesn't seem right to me.

Mon, Nov 12, 1:33 PM

Fri, Oct 26

arphaman accepted D53621: Support for groups of attributes in #pragma clang attribute.

LGTM too when Aaron's comments are addressed

Fri, Oct 26, 2:11 PM

Thu, Oct 25

arphaman added a comment to D53621: Support for groups of attributes in #pragma clang attribute.

Thanks! It looks pretty good, just minor nit. could you also update the language extension doc and release notes?

Thu, Oct 25, 3:42 PM

Oct 18 2018

arphaman added a comment to D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype.

With regards to D53352: you can change the diff related to a patch whilst keeping discuccion and metadata of the diff.

Oct 18 2018, 4:11 PM
arphaman updated the summary of D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype.
Oct 18 2018, 11:38 AM
arphaman added inline comments to D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype.
Oct 18 2018, 11:03 AM
arphaman added inline comments to D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype.
Oct 18 2018, 10:54 AM
arphaman added inline comments to D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype.
Oct 18 2018, 10:40 AM

Oct 16 2018

arphaman created D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype.
Oct 16 2018, 6:48 PM
arphaman abandoned D53352: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype.

Oops, this is a wrong patch. Sorry for the noise.

Oct 16 2018, 6:47 PM
arphaman created D53352: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype.
Oct 16 2018, 6:46 PM

Sep 10 2018

arphaman accepted D51189: [Sema][ObjC] Infer availability of +new from availability of -init.

LGTM

Sep 10 2018, 1:49 PM
arphaman added a comment to D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client.

Sorry for the delayed response. It seems we absolutely need this if mirroring libclang is an absolute requirement.
We seem to have multiple implementation options:

Which classes to use for representing diagnostics? We can:

  1. Reuse existing hierarchy for diagnostics. (current option)
  2. Have separate hierarchies for "flat" (fixits with notes) and "grouped" (fix-its are always attached to main diag). "flat" diagnostics can be stored exactly as they come from clang, with notes and main diags in the same vector, main diag should lack the "notes" field. I.e. if we choose to go with "raw" clang mode, we can as well avoid grouping the notes altogether instead of having code that does something in between the classical libclang and new clangd approach.

    I think the added separation is worth it for code clarity, but also realize it's a bunch of boilerplate, so others may disagree. WDYT?
Sep 10 2018, 1:47 PM
arphaman added inline comments to D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.
Sep 10 2018, 12:59 PM
arphaman updated the diff for D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.

Remove dead code for filesystem update fileID matching.

Sep 10 2018, 12:59 PM

Sep 7 2018

arphaman committed rC341697: warn_stdlibcxx_not_found: suggest '-stdlib=libc++' instead of '-std'.
warn_stdlibcxx_not_found: suggest '-stdlib=libc++' instead of '-std'
Sep 7 2018, 12:04 PM
arphaman committed rL341697: warn_stdlibcxx_not_found: suggest '-stdlib=libc++' instead of '-std'.
warn_stdlibcxx_not_found: suggest '-stdlib=libc++' instead of '-std'
Sep 7 2018, 12:04 PM

Sep 4 2018

arphaman accepted D51649: [Sema] Don't warn about omitting unavailable enum constants in a switch.

LGTM.

Sep 4 2018, 5:24 PM
arphaman accepted D51507: Allow all supportable attributes to be used with #pragma clang attribute..

LGTM, Thank you!

Sep 4 2018, 1:50 PM

Aug 24 2018

arphaman accepted D50993: [clangd] Increase stack size of the new threads on macOS.
Aug 24 2018, 3:10 PM
arphaman added a comment to D51189: [Sema][ObjC] Infer availability of +new from availability of -init.

That's probably the best solution then, I don't think declaring implicit new just for availability attribute is sound.

Aug 24 2018, 10:32 AM

Aug 23 2018

arphaman updated subscribers of D51189: [Sema][ObjC] Infer availability of +new from availability of -init.

Hmm, I don't think this solution is ideal, we'd rather have an attribute somewhere for other consumers of availability annotations. Does MyObject have an implicit decl of new, or are we referring to NSObjects new? Ideally we would an attribute on a particular new instead, but that might not work.

We're referring to NSObject's new. I don't think it's unreasonable to ask users who override init to be unavailable also override new with the same annotation, but it seems like extra boilerplate for something that we can easily infer in clang. What other consumers are you concerned about?

Aug 23 2018, 5:40 PM
arphaman added reviewers for D50318: Support Swift in platform availability attribute: arphaman, jfb.
Aug 23 2018, 5:30 PM
arphaman added a comment to D51189: [Sema][ObjC] Infer availability of +new from availability of -init.

Hmm, I don't think this solution is ideal, we'd rather have an attribute somewhere for other consumers of availability annotations. Does MyObject have an implicit decl of new, or are we referring to NSObjects new? Ideally we would an attribute on a particular new instead, but that might not work.

Aug 23 2018, 4:33 PM

Aug 22 2018

arphaman added inline comments to D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.
Aug 22 2018, 4:47 PM
arphaman updated the diff for D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.

Address review comments.

Aug 22 2018, 4:47 PM
arphaman added a comment to D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category.

LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.

This isn't in the spec and broke the LSP client eglot (https://github.com/joaotavora/eglot/pull/81). Why don't you put this in the "source" field, or concat it to the "message" field. Who can even use this information if it's not in the spec? Are clients supposed to code against every LSP server's whim?

Thanks for the feedback. I'll make a patch that turns this off by default so that clients can opt-in into it.

Thank you very much, and sorry if I came across a bit hostile. What is this category field good for?

Aug 22 2018, 1:31 PM
arphaman committed rCTE340449: [clangd] send diagnostic categories only when 'categorySupport'.
[clangd] send diagnostic categories only when 'categorySupport'
Aug 22 2018, 1:31 PM
arphaman committed rL340449: [clangd] send diagnostic categories only when 'categorySupport'.
[clangd] send diagnostic categories only when 'categorySupport'
Aug 22 2018, 1:31 PM
arphaman closed D51077: [clangd] send diagnostic categories only when 'categorySupport' capability was given by the client.
Aug 22 2018, 1:31 PM

Aug 21 2018

arphaman created D51077: [clangd] send diagnostic categories only when 'categorySupport' capability was given by the client.
Aug 21 2018, 5:38 PM
arphaman added a comment to D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client.

Just to make sure I fully understand the use-case: could you elaborate a little more? Do you need to get exactly the same set of notes that clang provides?

Aug 21 2018, 5:00 PM
arphaman added inline comments to D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.
Aug 21 2018, 4:50 PM
arphaman updated the diff for D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.

Address review comments

Aug 21 2018, 4:49 PM
arphaman added inline comments to D50993: [clangd] Increase stack size of the new threads on macOS.
Aug 21 2018, 3:23 PM

Aug 20 2018

arphaman committed rT340196: [SingleSource] update the protocols.m test after r340102.
[SingleSource] update the protocols.m test after r340102
Aug 20 2018, 11:56 AM
arphaman committed rL340196: [SingleSource] update the protocols.m test after r340102.
[SingleSource] update the protocols.m test after r340102
Aug 20 2018, 11:42 AM
arphaman added inline comments to D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.
Aug 20 2018, 10:17 AM
arphaman updated the diff for D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.

Address review comments.

Aug 20 2018, 10:16 AM
arphaman added a comment to D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category.

LGTM. Let's watch out for possible breakages in any of the clients, though. I've checked VSCode and it seems to be fine with the added fields.

This isn't in the spec and broke the LSP client eglot (https://github.com/joaotavora/eglot/pull/81). Why don't you put this in the "source" field, or concat it to the "message" field. Who can even use this information if it's not in the spec? Are clients supposed to code against every LSP server's whim?

Aug 20 2018, 9:40 AM

Aug 17 2018

arphaman committed rC340102: [ObjC] Error out when using forward-declared protocol in a @protocol.
[ObjC] Error out when using forward-declared protocol in a @protocol
Aug 17 2018, 3:19 PM
arphaman committed rL340102: [ObjC] Error out when using forward-declared protocol in a @protocol.
[ObjC] Error out when using forward-declared protocol in a @protocol
Aug 17 2018, 3:19 PM
arphaman closed D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression.
Aug 17 2018, 3:18 PM
arphaman added a comment to D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client.

Our client has a presentation layer with a Clang-based hierarchical model for diagnostics, so we need the original notes. This is a hard requirement in that sense.

Aug 17 2018, 2:24 PM
arphaman abandoned D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness.
Aug 17 2018, 2:19 PM
arphaman added a comment to D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness.

Hmm, after more analysis I realized that this is not the right solution for the rename problem. It would be correct to map one file:line:column location to a set of SourceLocations, and to use the old isPointWithin on a set of such locations. I opened https://reviews.llvm.org/D50926 instead. Sorry for the trouble!

Aug 17 2018, 2:18 PM
arphaman created D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.
Aug 17 2018, 2:18 PM
arphaman added inline comments to D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression.
Aug 17 2018, 12:52 PM
arphaman added inline comments to D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression.
Aug 17 2018, 11:19 AM

Aug 16 2018

arphaman added a comment to D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression.

Sorry for the delay.

Aug 16 2018, 4:30 PM
arphaman updated the diff for D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression.

address review comments.

Aug 16 2018, 4:30 PM
arphaman updated the diff for D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness.
  • Use lambda
  • Work with spelling locs
Aug 16 2018, 3:29 PM
arphaman added inline comments to D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness.
Aug 16 2018, 3:28 PM
arphaman added a comment to D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness.

Hi Alex, nice work!

I am just wondering if it would be beneficial to assert Loc and End are in the same file. It might help to catch bugs.

Aug 16 2018, 1:59 PM

Aug 15 2018

arphaman created D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client.
Aug 15 2018, 3:17 PM
arphaman added a dependency for D50801: [rename] Use isPointWithin when looking for a declaration at location: D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness.
Aug 15 2018, 1:08 PM
arphaman added a dependent revision for D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness: D50801: [rename] Use isPointWithin when looking for a declaration at location.
Aug 15 2018, 1:08 PM
arphaman created D50801: [rename] Use isPointWithin when looking for a declaration at location.
Aug 15 2018, 1:08 PM
arphaman added a comment to D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode.

I think propagating the 'test' yes/no value is not the best way to describe the intention of this change.
Our intention is to exit from the LSP server on JSON error. One way to describe this intention better is to propagate a boolean 'exitOnJSONError' value.
Furthermore, If you think about the '-llvm-lit' flag itself you'll see that it acts an abbreviation for these 3 options: -input-style=delimited -pretty -run-synchronously. I think that it would be valuable to follow this intention and to add a new option (e.g. -exit-on-json-error) that will then be set when -llvm-lit is set.
WDYT?

Aug 15 2018, 10:50 AM · Restricted Project

Aug 14 2018

arphaman committed rCTE339739: [clangd] update the new test to check for diagnostic's category as well.
[clangd] update the new test to check for diagnostic's category as well
Aug 14 2018, 3:27 PM
arphaman committed rL339739: [clangd] update the new test to check for diagnostic's category as well.
[clangd] update the new test to check for diagnostic's category as well
Aug 14 2018, 3:27 PM
arphaman committed rL339738: [clangd] add an extension field to LSP to transfer the diagnostic's category.
[clangd] add an extension field to LSP to transfer the diagnostic's category
Aug 14 2018, 3:22 PM
arphaman committed rCTE339738: [clangd] add an extension field to LSP to transfer the diagnostic's category.
[clangd] add an extension field to LSP to transfer the diagnostic's category
Aug 14 2018, 3:22 PM
arphaman closed D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category.
Aug 14 2018, 3:22 PM
arphaman committed rL339737: [clangd] add missing test from r339454.
[clangd] add missing test from r339454
Aug 14 2018, 3:21 PM
arphaman committed rCTE339737: [clangd] add missing test from r339454.
[clangd] add missing test from r339454
Aug 14 2018, 3:21 PM
arphaman created D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness.
Aug 14 2018, 2:56 PM
arphaman accepted D50618: Refactor Darwin driver to refer to runtimes by component.
Aug 14 2018, 1:29 PM

Aug 13 2018

arphaman added a comment to D50641: [clangd][test] Fix exit messages in tests.

Thanks for fixing this!

Aug 13 2018, 3:19 PM · Restricted Project
arphaman updated the diff for D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category.

Address review comments.

Aug 13 2018, 3:14 PM

Aug 10 2018

arphaman created D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category.
Aug 10 2018, 10:37 AM
arphaman committed rL339454: [clangd] extend the publishDiagnostics response to send back fixits to the.
[clangd] extend the publishDiagnostics response to send back fixits to the
Aug 10 2018, 10:25 AM
arphaman committed rCTE339454: [clangd] extend the publishDiagnostics response to send back fixits to the.
[clangd] extend the publishDiagnostics response to send back fixits to the
Aug 10 2018, 10:25 AM
arphaman closed D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested).
Aug 10 2018, 10:25 AM

Aug 9 2018

arphaman updated the diff for D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested).

remove parameter.

Aug 9 2018, 2:21 PM

Aug 8 2018

arphaman added a comment to D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected.

On a second look I think that there is value separating the concepts of 'producing diagnostics' after hitting the fatal error (which is SuppressAfterFatalError currently does), and trying to build a more complete AST.
For example,

  • An editor client might not want to show the diagnostics after the fatal error has been reached to match the diagnostics observed during build, but it would benefit from a more complete AST for other editing features.
  • Index-while-building: the compiler shouldn't show the diagnostics after a fatal error has been reached, but the indexer would be able to produce better indexing data from a more complete AST.
Aug 8 2018, 4:26 PM
arphaman added a comment to D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected.

This seems sensible to me.

Aug 8 2018, 4:22 PM
arphaman updated the diff for D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested).
  • Client now can request the fixes using a 'clientCapabilities/textDocument/publishDiagnostics' extension.
  • Use 'Fixes' instead of 'Fixits' in code.
Aug 8 2018, 11:39 AM
arphaman committed rCRT339277: [macOS] stop generating the libclang_rt.10.4.a library for macOS 10.4.
[macOS] stop generating the libclang_rt.10.4.a library for macOS 10.4
Aug 8 2018, 10:30 AM
arphaman committed rL339277: [macOS] stop generating the libclang_rt.10.4.a library for macOS 10.4.
[macOS] stop generating the libclang_rt.10.4.a library for macOS 10.4
Aug 8 2018, 10:30 AM

Aug 7 2018

arphaman created D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested).
Aug 7 2018, 3:53 PM
arphaman added a comment to D50154: [clangd] capitalize diagnostic messages.

What's the motivation for clangd to differ from clang here? (& if the first
letter is going to be capitalized, should there be a period at the end? But
also the phrasing of most/all diagnostic text isn't in the form of complete
sentences, so this might not make sense)

Aug 7 2018, 10:33 AM

Aug 3 2018

arphaman committed rL338919: [clangd] capitalize diagnostic messages.
[clangd] capitalize diagnostic messages
Aug 3 2018, 1:44 PM
arphaman committed rCTE338919: [clangd] capitalize diagnostic messages.
[clangd] capitalize diagnostic messages
Aug 3 2018, 1:44 PM
arphaman closed D50154: [clangd] capitalize diagnostic messages.
Aug 3 2018, 1:43 PM
arphaman retitled D50154: [clangd] capitalize diagnostic messages from [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided to [clangd] capitalize diagnostic messages.
Aug 3 2018, 12:38 PM
arphaman accepted D50255: Add test for changing build configuration.

Thanks! This LGTM.

Aug 3 2018, 12:36 PM
arphaman added inline comments to D50154: [clangd] capitalize diagnostic messages.
Aug 3 2018, 11:18 AM