arphaman (Alex Lorenz)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Mon, Sep 10

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

LGTM

Mon, Sep 10, 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?
Mon, Sep 10, 1:47 PM
arphaman added inline comments to D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.
Mon, Sep 10, 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.

Mon, Sep 10, 12:59 PM

Fri, Sep 7

arphaman committed rC341697: warn_stdlibcxx_not_found: suggest '-stdlib=libc++' instead of '-std'.
warn_stdlibcxx_not_found: suggest '-stdlib=libc++' instead of '-std'
Fri, Sep 7, 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'
Fri, Sep 7, 12:04 PM

Tue, Sep 4

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

LGTM.

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

LGTM, Thank you!

Tue, Sep 4, 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
arphaman updated the diff for D50154: [clangd] capitalize diagnostic messages.

Remove test and update unit test.

Aug 3 2018, 11:18 AM
arphaman updated the diff for D50154: [clangd] capitalize diagnostic messages.

Always capitalize the diagnostic's message.

Aug 3 2018, 10:19 AM

Aug 1 2018

arphaman created D50154: [clangd] capitalize diagnostic messages.
Aug 1 2018, 12:36 PM
arphaman committed rCTE338597: [clangd] allow clients to control the compilation database by passing in.
[clangd] allow clients to control the compilation database by passing in
Aug 1 2018, 10:40 AM
arphaman committed rL338597: [clangd] allow clients to control the compilation database by passing in.
[clangd] allow clients to control the compilation database by passing in
Aug 1 2018, 10:40 AM
arphaman closed D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.
Aug 1 2018, 10:40 AM · Restricted Project

Jul 31 2018

arphaman added inline comments to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.
Jul 31 2018, 1:38 PM · Restricted Project
arphaman updated the diff for D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.

Updated to address review comments.

Jul 31 2018, 1:37 PM · Restricted Project

Jul 30 2018

arphaman added inline comments to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.
Jul 30 2018, 11:28 AM · Restricted Project
arphaman updated the diff for D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.

Address review comments

Jul 30 2018, 11:28 AM · Restricted Project

Jul 27 2018

arphaman requested changes to D49548: [clangd] XPC WIP.

FYI, this patch can't be applied because of the broken file paths (some diffs include /clangd prefix, some don't, while the test diffs are missing '/test').

Jul 27 2018, 5:21 PM
arphaman updated the diff for D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.

Updated patch to address review comments:

  • The compilation database updated are used only when '-in-memory-compile-commands' flag is used.
  • It's now possible to set the working directory as well.
Jul 27 2018, 2:06 PM · Restricted Project
arphaman added a comment to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

The client will control the commands exclusively.

Maybe a cleaner design would be to untangle the two use-cases and control them with a flag to clangd?
I.e. we can have two implementations of compilation databases in clangd:

  • one that uses clang tooling capabilities, i.e. reads compile_commands.json, etc.
  • one that gets all compile commands from the protocol and won't use the clang tooling. The command-line arg to clangd will control which implementation is used.

    The advantage is that we don't have to think about interactions between the clang plugins and explicit overrides and it should be easier to make sure that we don't accidentally read compilation args from the wrong place. Would also help to keep DirectoryBasedCompilationDatabase a bit simpler, and the other implementation would be extremely simple. WDYT?
Jul 27 2018, 10:25 AM · Restricted Project

Jul 26 2018

arphaman added inline comments to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.
Jul 26 2018, 7:16 PM · Restricted Project
arphaman added a comment to D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request.

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

Jul 26 2018, 7:15 PM · Restricted Project
arphaman abandoned D49523: [clangd] Add support for per-file override compilation command.

Abandoned in favor of https://reviews.llvm.org/D49758 (will update it today).

Jul 26 2018, 8:52 AM · Restricted Project
arphaman added a comment to D49523: [clangd] Add support for per-file override compilation command.

We actually need both mechanisms. I posted the didChangeConfiguration extension to https://reviews.llvm.org/D49758.

Why do we need both? Can we have only the one from D49758 and send two requests (compile command changed for file 'X' + didOpen('X')) whenever a new file gets added?

Jul 26 2018, 8:52 AM · Restricted Project
arphaman added a comment to D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one.

Sounds good

Jul 26 2018, 8:47 AM

Jul 25 2018

arphaman committed rL337997: Revert r337981: it breaks the debuginfo-tests.
Revert r337981: it breaks the debuginfo-tests
Jul 25 2018, 8:22 PM
arphaman committed rCXX337984: [libc++] Follow-up to r337968: use an explicit cast as suggested by Eric.
[libc++] Follow-up to r337968: use an explicit cast as suggested by Eric
Jul 25 2018, 5:00 PM
arphaman committed rL337984: [libc++] Follow-up to r337968: use an explicit cast as suggested by Eric.
[libc++] Follow-up to r337968: use an explicit cast as suggested by Eric
Jul 25 2018, 5:00 PM
arphaman committed rL337968: [libc++] Follow-up to r337960: specify lambda's return type to avoid.
[libc++] Follow-up to r337960: specify lambda's return type to avoid
Jul 25 2018, 2:51 PM
arphaman committed rCXX337968: [libc++] Follow-up to r337960: specify lambda's return type to avoid.
[libc++] Follow-up to r337960: specify lambda's return type to avoid
Jul 25 2018, 2:51 PM