Page MenuHomePhabricator

njames93 (Nathan James)
Dodd

Projects

User does not belong to any projects.

User Details

User Since
Dec 23 2019, 11:05 AM (55 w, 5 d)

Recent Activity

Today

njames93 requested changes to D94131: [clang-tidy] Use new mapAnyOf matcher.

Can you either update the description of this patch to include the binaryOperation changes, or remove those changes from here.

Sat, Jan 16, 1:27 PM · Restricted Project

Yesterday

njames93 added a comment to D94785: [clangd] Index local classes, virtual and overriding methods..

Thank you for taking the time to fix this and the performance measurements to boot.
My only question is does this try and index lambdas, under the hood they are declared as a CXXRecordDecl, defined in function scope?

Fri, Jan 15, 8:55 AM · Restricted Project

Thu, Jan 14

njames93 added a reviewer for D94621: [clang-tidy] add concurrency-async-fs: aaron.ballman.

Is this just flagging all these functions, if so I don't think there's much value in here.

Thu, Jan 14, 4:40 AM · Restricted Project, Restricted Project
njames93 accepted D94601: [clang-tidy] Use DenseSet<SourceLocation> in UpgradeDurationConversionsCheck, NFCI.

Seems reasonable, LGTM.

Thu, Jan 14, 4:37 AM · Restricted Project, Restricted Project
njames93 updated the diff for D94554: [clangd][WIP] Add a Filesystem that overlays Dirty files..

Refactor RefCntString out of DraftStore class
Remove MTime from Draft, other users of DraftStore don't need to see it.

Thu, Jan 14, 2:28 AM · Restricted Project

Wed, Jan 13

njames93 added a comment to D93452: [clangd] Trim memory periodically.

A little follow up, but I've noticed if I have a lot of tabs open in my editor (usually happens as I forget to close them).
When clangd starts (or restarts) , in the first minute, memory usage will shoot right up as it starts to do its thing.
I've regularly seen it go over 15GB before the first trim call is made and it drops back down and settling at ~2GB. For reference clangd typically reports its usage is ~1.5GB when I'm working on LLVM.
This does result in some noticeable hanging of my system, which while not workstation level, its got more horsepower than a chromebook, no offence to anyone who works at Google :).

Wed, Jan 13, 6:21 PM · Restricted Project, Restricted Project
njames93 committed rGaf1bb4bc823f: Fix build errors after ceb9379a9 (authored by njames93).
Fix build errors after ceb9379a9
Wed, Jan 13, 4:20 AM
njames93 committed rGceb9379a90f5: [ADT] Fix join_impl using the wrong size when calculating total length (authored by njames93).
[ADT] Fix join_impl using the wrong size when calculating total length
Wed, Jan 13, 3:37 AM
njames93 closed D83305: [ADT] Fix join_impl using the wrong size when calculating total length.
Wed, Jan 13, 3:37 AM · Restricted Project
njames93 updated the diff for D94554: [clangd][WIP] Add a Filesystem that overlays Dirty files..

Remove some unrelated changes

Wed, Jan 13, 3:35 AM · Restricted Project

Tue, Jan 12

njames93 updated the diff for D83305: [ADT] Fix join_impl using the wrong size when calculating total length.

Add assert to enusre correct behaviour.

Tue, Jan 12, 3:57 PM · Restricted Project
njames93 added a comment to D83305: [ADT] Fix join_impl using the wrong size when calculating total length.

The spec seems to be a bit more precise than that (using cppreference, which isn't authoritative, but more readable): "If new_cap is greater than the current capacity(), new storage is allocated, and capacity() is made equal or greater than new_cap."

So a unit test that joins some large strings (if it wanted to be fancy, it could even generate a string intentionally larger than std::string's default capacity, whatever it happens to be) and if the resulting string has a larger capacity than std::string's default capacity, verify that the capacity is exactly equal to the string length.

That still sounds like it'll cause some issues. The main thing we want to test (which is the purpose of this patch) is that the string didn't grow while we are appending all the pieces. It's impossible to tell externally if that happened as reserve may choose to over allocate.
Putting an assert in the function that the capacity before hasn't changed should be more than enough, WDYT?

Tue, Jan 12, 3:56 PM · Restricted Project
njames93 committed rGa7130d85e4b9: [ADT][NFC] Use empty base optimisation in BumpPtrAllocatorImpl (authored by njames93).
[ADT][NFC] Use empty base optimisation in BumpPtrAllocatorImpl
Tue, Jan 12, 2:44 PM
njames93 closed D94439: [ADT][NFC] Use empty base optimisation in BumpPtrAllocatorImpl.
Tue, Jan 12, 2:44 PM · Restricted Project
njames93 planned changes to D94554: [clangd][WIP] Add a Filesystem that overlays Dirty files..
Tue, Jan 12, 2:37 PM · Restricted Project
njames93 requested review of D94554: [clangd][WIP] Add a Filesystem that overlays Dirty files..
Tue, Jan 12, 2:20 PM · Restricted Project

Mon, Jan 11

njames93 updated the summary of D94439: [ADT][NFC] Use empty base optimisation in BumpPtrAllocatorImpl.
Mon, Jan 11, 1:05 PM · Restricted Project
njames93 added a comment to D94439: [ADT][NFC] Use empty base optimisation in BumpPtrAllocatorImpl.

I think for full generality the standard library does some things to check if a type can be derived from before using it in this way - but we probably don't need all that machinery for LLVM-internal allocator stuff like this?

We just have to make sure people don't use an allocator that's defined with the final keyword. I doubt we are going to run into many cases of that.
I have done the template magic to get it make it work if someone wants to use an allocator marked final, should I include it?

Probably wouldn't bother - might be worth leaving it here or in the commit message, or in a comment in the code as a "here's how to do it if it turns out someone needs it".

To make the class work is code than I'd be happy to tack onto the commit message. I'll just make a note in the commit message about it.
Basically just create a class that has an instance of the final Allocator type, then create calls that forward to that instance.

Mon, Jan 11, 1:01 PM · Restricted Project
njames93 added a comment to D94439: [ADT][NFC] Use empty base optimisation in BumpPtrAllocatorImpl.

I think for full generality the standard library does some things to check if a type can be derived from before using it in this way - but we probably don't need all that machinery for LLVM-internal allocator stuff like this?

We just have to make sure people don't use an allocator that's defined with the final keyword. I doubt we are going to run into many cases of that.
I have done the template magic to get it make it work if someone wants to use an allocator marked final, should I include it?

Mon, Jan 11, 12:50 PM · Restricted Project
njames93 committed rGd3ff24cbf872: [ADT] Add makeIntrusiveRefCnt helper function (authored by njames93).
[ADT] Add makeIntrusiveRefCnt helper function
Mon, Jan 11, 12:13 PM
njames93 closed D94440: [ADT] Add makeIntrusiveRefCnt helper function.
Mon, Jan 11, 12:13 PM · Restricted Project
njames93 updated the diff for D94440: [ADT] Add makeIntrusiveRefCnt helper function.

Address comments.

Mon, Jan 11, 12:12 PM · Restricted Project
njames93 added a comment to D94390: [clangd] Extend find-refs to include overrides..

Should we be providing virtual method overrides when querying definitions, surely that's what query implementations is for?

Mon, Jan 11, 12:03 PM · Restricted Project
njames93 added inline comments to D94440: [ADT] Add makeIntrusiveRefCnt helper function.
Mon, Jan 11, 11:49 AM · Restricted Project
njames93 requested review of D94440: [ADT] Add makeIntrusiveRefCnt helper function.
Mon, Jan 11, 11:48 AM · Restricted Project
njames93 requested review of D94439: [ADT][NFC] Use empty base optimisation in BumpPtrAllocatorImpl.
Mon, Jan 11, 11:27 AM · Restricted Project
njames93 committed rG31732e6f52c8: [clangd] Remove ScratchFS from tests (authored by njames93).
[clangd] Remove ScratchFS from tests
Mon, Jan 11, 8:14 AM
njames93 closed D94359: [clangd] Remove ScratchFS from tests.
Mon, Jan 11, 8:14 AM · Restricted Project
njames93 updated the summary of D94359: [clangd] Remove ScratchFS from tests.
Mon, Jan 11, 2:08 AM · Restricted Project
njames93 updated the diff for D94359: [clangd] Remove ScratchFS from tests.

Remove extra semi-colon.

Mon, Jan 11, 2:08 AM · Restricted Project

Sun, Jan 10

njames93 added inline comments to D94382: [clangd] Avoid recursion in TargetFinder::add().
Sun, Jan 10, 6:06 PM · Restricted Project
njames93 retitled D94382: [clangd] Avoid recursion in TargetFinder::add() from [clangd] Avoid recusion in TargetFinder::add() to [clangd] Avoid recursion in TargetFinder::add().
Sun, Jan 10, 5:56 PM · Restricted Project
njames93 added a comment to D93978: [clangd] DefineOutline doesn't require implementation file being saved.

So I did some tweaking and I have a snapshot filesystem that will hold shared references to the contents, thereby avoiding the need to copy the file contents.
It works for rename and tweaks with no visible issues.
Reasoning behind this a long term goal of letting a user choose between using on disk or dirty buffers when building preambles.
Obviously there, if using dirty buffers, we wouldn't want to be copying every open file (even unused files) during preamble building/checking.

Sun, Jan 10, 4:56 PM · Restricted Project

Sat, Jan 9

njames93 updated the diff for D94359: [clangd] Remove ScratchFS from tests.

Fix windows tests failing(hopefully).

Sat, Jan 9, 5:28 AM · Restricted Project
njames93 requested review of D94359: [clangd] Remove ScratchFS from tests.
Sat, Jan 9, 4:45 AM · Restricted Project

Fri, Jan 8

njames93 committed rG467cbd298184: [clangd][NFC] Remove unnecessary copy in CodeComplete (authored by njames93).
[clangd][NFC] Remove unnecessary copy in CodeComplete
Fri, Jan 8, 6:32 PM
njames93 added a comment to D94293: [clangd] automatically index STL.

How does this approach work with differing language standards. For example with an old implementation designed for c++14, string_view header won't exist. If the implementation is designed to work with c++17, including that header in c++14 mode will probably be ifdef... Error.

Fri, Jan 8, 4:55 AM · Restricted Project

Thu, Jan 7

njames93 updated the diff for D93978: [clangd] DefineOutline doesn't require implementation file being saved.

Address most comments.

Thu, Jan 7, 2:01 PM · Restricted Project
njames93 added inline comments to D93978: [clangd] DefineOutline doesn't require implementation file being saved.
Thu, Jan 7, 1:59 PM · Restricted Project
njames93 updated the summary of D93978: [clangd] DefineOutline doesn't require implementation file being saved.
Thu, Jan 7, 1:55 AM · Restricted Project
njames93 updated the diff for D93978: [clangd] DefineOutline doesn't require implementation file being saved.

Use VFS instead of DraftStore

Thu, Jan 7, 1:55 AM · Restricted Project
njames93 abandoned D93977: [clangd] Pass DraftStore to Tweak Apply and Prepare..

Abandoning in favour of VFS approach

Thu, Jan 7, 1:26 AM · Restricted Project

Wed, Jan 6

njames93 added a comment to D93978: [clangd] DefineOutline doesn't require implementation file being saved.

Ah, thanks for working on this!

A few thoughts:

  • when we're pseudoparsing the file we're going to modify as we do here, using the new content is strictly better, no downside :-)
  • the old method here of accessing the content through the FS attached to the AST is a hack
  • however I think DraftStore is the wrong abstraction here and VFS is the right one.
    • each tweak shouldn't have to deal with falling back from DraftStore to VFS (and rename also has an implementation of this!)
    • C++ embedders don't have a DraftStore lying around
    • bug: this code won't find a named but unsaved implementation file (since getCorrespondingHeaderOrSource uses VFS)
  • (also Tweak::Selection isn't a pure abstraction, we can jam the FS in there instead of passing it around as a separate param everywhere)

So I think the right way to expose this to tweaks is to add a vfs::FileSystem * member to Tweak::Selection, that allows accessing the current FS (as opposed to the one used for building).
The simplest implementation uses OverlayVFS + InMemoryFS + TUScheduler::getAllFileContents(). This isn't as expensive as it sounds as we need only set the FS pointer for apply().
(We can also implement a FS that copies more lazily, though I suspect it's not worth it)

Wed, Jan 6, 6:52 PM · Restricted Project
njames93 committed rG3505d8dc0742: [clangd][NFC] Use PathRef for getCorrespondingHeaderOrSource (authored by njames93).
[clangd][NFC] Use PathRef for getCorrespondingHeaderOrSource
Wed, Jan 6, 6:42 PM
njames93 committed rG0bfe10014563: [NFC] Test case refactor (authored by njames93).
[NFC] Test case refactor
Wed, Jan 6, 12:00 PM
njames93 added a comment to D94131: [clang-tidy] Use new mapAnyOf matcher.

Theres a few compile errors here in the pre merge. Is this patch based against trunk or some local branch?

Wed, Jan 6, 3:54 AM · Restricted Project

Sun, Jan 3

njames93 added a comment to D93979: [clang-tidy] Fix windows tests.

http://45.33.8.238/win/30678/summary.html It looks like it works, as nothing needed to be built it was a v fast build

Sun, Jan 3, 4:44 PM · Restricted Project
njames93 committed rG59810c51e761: [clang-tidy] Fix windows tests (authored by njames93).
[clang-tidy] Fix windows tests
Sun, Jan 3, 4:40 PM
njames93 closed D93979: [clang-tidy] Fix windows tests.
Sun, Jan 3, 4:39 PM · Restricted Project

Sat, Jan 2

njames93 added a comment to D93979: [clang-tidy] Fix windows tests.

I don't have a (reliable) windows machine to test so can you take a look please

Sat, Jan 2, 3:54 PM · Restricted Project
njames93 requested review of D93979: [clang-tidy] Fix windows tests.
Sat, Jan 2, 3:53 PM · Restricted Project
njames93 committed rG7af6a134508c: [NFC] Switch up some dyn_cast calls (authored by njames93).
[NFC] Switch up some dyn_cast calls
Sat, Jan 2, 11:57 AM
njames93 requested review of D93978: [clangd] DefineOutline doesn't require implementation file being saved.
Sat, Jan 2, 10:14 AM · Restricted Project
njames93 requested review of D93977: [clangd] Pass DraftStore to Tweak Apply and Prepare..
Sat, Jan 2, 10:04 AM · Restricted Project

Wed, Dec 30

njames93 added inline comments to D93844: [clang-format] Add possibility to be based on parent directory.
Wed, Dec 30, 6:56 AM · Restricted Project, Restricted Project
njames93 added a comment to D93940: [clang-tidy] Add a check for blocking types and functions..

Few stylistic nits, Also Theres lots of cases where single stmt if statements have braces, typically we elide those braces.
Is it worth flagging methods with Thread safety analysis attributes.
These are only used in clang, but if a project annotates their methods with these, it would be nice to autodetect these attributes, though I can see this being a big job and likely better for a follow up.

Wed, Dec 30, 5:14 AM · Restricted Project, Restricted Project

Tue, Dec 29

njames93 added a comment to D83305: [ADT] Fix join_impl using the wrong size when calculating total length.

Any chance of testing this? Could test the 'capacity' after join to check its the right size?

How do you suppose I do that, maybe expensive checks to get the capacity after reserve and after all items have been added and ensure they are the same, otherwise the string grew during appending.

Oh, nah, nothing that'd require expensive checks - you could add an assert that size == Len at the end of the function (would have to have some way of avoiding the case for short results that are below the starting capacity, though... perhaps checking if capacity grew over the reserve call, and only asserting in that case)). But I was thinking more along the lines of a unit test that tests join and checks the capacity isn't bigger than the size for a few hardcoded samples.

Oh that assert is fine, the unit test won't be a good test. As we don't control std string, it's capacity for calls to reserve will be implementation defined

Tue, Dec 29, 3:30 PM · Restricted Project
njames93 added a comment to D83305: [ADT] Fix join_impl using the wrong size when calculating total length.

Any chance of testing this? Could test the 'capacity' after join to check its the right size?

Tue, Dec 29, 3:14 PM · Restricted Project

Mon, Dec 28

njames93 added a reviewer for D83305: [ADT] Fix join_impl using the wrong size when calculating total length: dblaikie.

Ping.

Mon, Dec 28, 1:11 PM · Restricted Project
njames93 accepted D93653: [clangd] Avoid reallocating buffers for each message read:.

I'm easy with this.

Mon, Dec 28, 12:07 PM · Restricted Project
njames93 updated the diff for D93861: [clang-tidy][NFC] Split up some headers.

Fix clang-tidy lint and comments.

Mon, Dec 28, 11:43 AM · Restricted Project
njames93 requested review of D93861: [clang-tidy][NFC] Split up some headers.
Mon, Dec 28, 8:18 AM · Restricted Project
njames93 committed rGc3b9d85bd4b7: [clang-tidy][NFC] Remove unnecessary headers (authored by njames93).
[clang-tidy][NFC] Remove unnecessary headers
Mon, Dec 28, 7:02 AM

Sun, Dec 27

njames93 added a comment to D93844: [clang-format] Add possibility to be based on parent directory.

I'm a little confused why this is needed as clang-format always read up the directory tree until it see a .clang-format file, perhaps I don't quite understand the use case. Can't you just have a different .clang-format in the subdirectory?

Sun, Dec 27, 3:12 PM · Restricted Project, Restricted Project

Sat, Dec 26

njames93 committed rG62beac7ed7c6: [NFC] Refactor some SourceMgr code (authored by njames93).
[NFC] Refactor some SourceMgr code
Sat, Dec 26, 9:54 AM
njames93 updated njames93.
Sat, Dec 26, 9:17 AM

Thu, Dec 24

njames93 requested review of D93800: [clangd][WIP] Add caching behaviour for clang-format config.
Thu, Dec 24, 3:26 AM · Restricted Project
njames93 added a comment to D93761: [libObject/Decompressor] - Use `resize_for_overwrite` in Decompressor::resizeAndDecompress()..

IMO this may be misleading. This appears to be faster because previously memset zero was called on the buffer which triggered page faults and allocates the pages immediately.
With the new change the time is amortized with the decompression itself. I think we should measure the total time with decompression.

Be interesting to see the difference, would be nice to query performance of decompression alone as well. See how much slowdown there is if pages haven't been allocated eagerly.

Thu, Dec 24, 1:47 AM · Restricted Project

Wed, Dec 23

njames93 added a comment to D93763: [clangd] Add a flag to disable the documentLinks LSP request.

Can you apply the clang-format patch suggested by the pre-merge bot. Alternatively, if you have a recent version of clang-format and the appropriate git-hooks you should be able to run git clang-format for the same effect.

Wed, Dec 23, 2:57 PM · Restricted Project
njames93 committed rGc7e825b910a9: [format][NFC] Use unsigned char as the base of all enums in FormatStyle (authored by njames93).
[format][NFC] Use unsigned char as the base of all enums in FormatStyle
Wed, Dec 23, 12:28 PM
njames93 closed D93758: [format][NFC] Use unsigned char as the base of all enums in FormatStyle.
Wed, Dec 23, 12:28 PM · Restricted Project
njames93 added a comment to D93758: [format][NFC] Use unsigned char as the base of all enums in FormatStyle.

I don't have any major issues with this other than it makes Format.h a bit more ugly. (there are more ugly and harder to understand pieces of code than this change!)

We'll only normally really have 1 of these in play at any one time, but it is passed around quite a bit on the stack, should we care about the extra space it's taking? (I'm ok if the answer is "yes we should")

Wed, Dec 23, 8:29 AM · Restricted Project
njames93 updated the diff for D93758: [format][NFC] Use unsigned char as the base of all enums in FormatStyle.
  • Update dump_format_style script

Only tweaked regex to support enum <name> : <words> ... {
Decided there wasn't much to gain from supporting nested types etc

Wed, Dec 23, 7:53 AM · Restricted Project
njames93 added a comment to D93758: [format][NFC] Use unsigned char as the base of all enums in FormatStyle.

We should probably check the docs/tools/dump_format_style.py still works

Good catch, I'll update the script to optionally take an underlying type argument. Should that be part of this change, or a parent?

Wed, Dec 23, 6:41 AM · Restricted Project
njames93 committed rGeb9483b21053: [format] Add overload to parseConfiguration that accept llvm::MemoryBufferRef (authored by njames93).
[format] Add overload to parseConfiguration that accept llvm::MemoryBufferRef
Wed, Dec 23, 4:08 AM
njames93 closed D93633: [format] Add overload to parseConfiguration that accept llvm::MemoryBufferRef.
Wed, Dec 23, 4:08 AM · Restricted Project
njames93 requested review of D93758: [format][NFC] Use unsigned char as the base of all enums in FormatStyle.
Wed, Dec 23, 4:02 AM · Restricted Project
njames93 published D93733: [NFC] replace resize calls with resize_for_overwrite for review.
Wed, Dec 23, 3:51 AM · Restricted Project, Restricted Project, Restricted Project

Tue, Dec 22

njames93 committed rGf5071489ea8c: [ADT] Fix some tests after 5d10b8ad (authored by njames93).
[ADT] Fix some tests after 5d10b8ad
Tue, Dec 22, 10:06 AM
njames93 committed rG5d10b8ad595d: [ADT] Add resize_for_overwrite method to SmallVector. (authored by njames93).
[ADT] Add resize_for_overwrite method to SmallVector.
Tue, Dec 22, 9:19 AM
njames93 closed D93532: [ADT] Add resize_for_overwrite method to SmallVector..
Tue, Dec 22, 9:19 AM · Restricted Project
njames93 accepted D91837: ADT: Fix reference invalidation in SmallVector APIs that pass in a value.

Great, thanks for doing that analysis @nikic!

@njames93, I'll land this once you accept.

LGTM, but I'm not the gatekeeper of ADT.

Tue, Dec 22, 9:18 AM · Restricted Project
njames93 added a comment to D93532: [ADT] Add resize_for_overwrite method to SmallVector..

LGTM. I'm hopeful we can somehow keep the tests once we instrument SmallVector for the sanitizers (see my comment below); even if not, I suppose we can drop the tests at that time.

When that happens, will there be a way to update this testcase, maybe to something like this?

V.push_back(5);
V.pop_back();
V.resize_for_overwrite(V.size() + 1);
if (!is_sanitizer_poisoning_pop_back())
  EXPECT_EQ(5, V.back());

or:

V.resize_for_overwrite(V.size() + 1);
if (is_sanitizer_poisoning_pop_back())
  EXPECT_TRUE(is_sanitizer_poison(V.back()));
else
  EXPECT_EQ(5, V.back());

We can detect MSAN using

#if LLVM_MEMORY_SANITIZER_BUILD
// We have msan, don't run tests.
#else
<The test code>
#endif

If these tests are causing issues under msan, then just don't run them.
As an added bonus if the tests are failing under msan, that would mean that msan will help catch bugs when people abuse this by not writing to the new storage before reading.

Tue, Dec 22, 3:36 AM · Restricted Project
njames93 updated the diff for D93532: [ADT] Add resize_for_overwrite method to SmallVector..

Period after comments.

Tue, Dec 22, 3:32 AM · Restricted Project
njames93 committed rG4b3633cf2cb6: [clangd] Reuse buffer for JSONTransport::sendMessage (authored by njames93).
[clangd] Reuse buffer for JSONTransport::sendMessage
Tue, Dec 22, 3:31 AM
njames93 closed D93531: [clangd] Reuse buffer for JSONTransport::sendMessage.
Tue, Dec 22, 3:31 AM · Restricted Project
njames93 retitled D93531: [clangd] Reuse buffer for JSONTransport::sendMessage from [clangd] Reuse buffer for JSONTransport::readRawMessage to [clangd] Reuse buffer for JSONTransport::sendMessage.
Tue, Dec 22, 3:29 AM · Restricted Project
njames93 updated the diff for D84924: [clang-tidy] Added command line option `fix-notes`.

Only apply fixes from notes if there is exactly one fix found in them.

Tue, Dec 22, 2:43 AM · Restricted Project, Restricted Project
njames93 added a comment to D84924: [clang-tidy] Added command line option `fix-notes`.

Agreed, but I don't think the current behavior in this patch is cautious enough. I like that we have to specify "please apply fixits from notes" explicitly. I think that's the right behavior we want. But if there are multiple fixits attached to the notes for a diagnostic, I don't think we should assume that the first fix-it is the correct one to apply -- I think the user should have to manually pick the variant they want in that case. So I think the behavior should be to apply the fixits from notes if there's only a single fixit to be applied. WDYT?

I can agree with that, One thing I like about using clangd is it will show you all available fixes, letting you pick and choose.

Tue, Dec 22, 2:10 AM · Restricted Project, Restricted Project
njames93 added a comment to D93452: [clangd] Trim memory periodically.

I feel like we need some kind of entry for this in release notes given how much of an issue it is.

Tue, Dec 22, 2:04 AM · Restricted Project, Restricted Project

Mon, Dec 21

njames93 added inline comments to D93633: [format] Add overload to parseConfiguration that accept llvm::MemoryBufferRef.
Mon, Dec 21, 3:56 PM · Restricted Project
njames93 requested review of D93633: [format] Add overload to parseConfiguration that accept llvm::MemoryBufferRef.
Mon, Dec 21, 6:22 AM · Restricted Project
njames93 added a comment to D93543: clang-tidy: Leave the possibility of opting out having coloured diagnostic messages..

Would it be wise to use os.isatty(sys.stdout.fileno()) as the value if left unspecified.
As we capture stdout from clang-tidy, clang-tidy assumes we aren't connected to a terminal and thus disables colours, unless explicitly enabled.
Therefore if our python script is known to be running from a terminal and we haven't specified to use colours. We should enable --use-color.

Mon, Dec 21, 4:56 AM · Restricted Project, Restricted Project
njames93 added a comment to D84924: [clang-tidy] Added command line option `fix-notes`.

This is very much a work in progress
Another direction I was thinking was only apply the fixes found in notes if there is exactly one fix attached to the notes in a diagnostic, instead of just applying the first one we find

I was wondering the same thing here myself. If there's exactly one fix, then it's unambiguous as to what behavior you get. One (minor) concern I have about the current approach is with nondeterminism in diagnostic ordering where different runs may pick different fixes for the same code. I don't *think* we have any instances of this in Clang or clang-tidy, but users can add their own plugins (for instance to the clang static analyzer) that may introduce some small risk there. Do you have a reason why you picked one approach over the other?

Part of the reason for this approach is from this bug report https://bugs.llvm.org/show_bug.cgi?id=47971, where its pointed out that clang diags gets fix-its added in notes if the fixes will change behaviour or they aren't sure its going to actually fix the issue.
As clang-tidy also applies fixes reported from clang, it is wise to adhere to a similar level caution.

Mon, Dec 21, 4:16 AM · Restricted Project, Restricted Project
njames93 retitled D84924: [clang-tidy] Added command line option `fix-notes` from [clang-tidy][WIP] Added command line option `fix-notes` to [clang-tidy] Added command line option `fix-notes`.
Mon, Dec 21, 4:07 AM · Restricted Project, Restricted Project
njames93 updated the diff for D84924: [clang-tidy] Added command line option `fix-notes`.

Use enum for handling fix behaviour.
Fix tests using --fix-notes instead of -fix-notes.

Mon, Dec 21, 4:06 AM · Restricted Project, Restricted Project

Sat, Dec 19

njames93 added a comment to D93452: [clangd] Trim memory periodically.

Just attached a debugger and ran malloc_trim on my clangd instance which was sitting at 6.7GB, afterwards it was 1.3GB. Interestingly clangd held rock solid at 1.0GB recorded memory usage throughout.

Sat, Dec 19, 6:21 PM · Restricted Project, Restricted Project
njames93 updated the diff for D93531: [clangd] Reuse buffer for JSONTransport::sendMessage.

Remove input buffers, but keep output as its easy to reason about.

Sat, Dec 19, 5:27 AM · Restricted Project
njames93 updated the diff for D93532: [ADT] Add resize_for_overwrite method to SmallVector..

Added test case, remove format artefact.

Sat, Dec 19, 3:00 AM · Restricted Project
njames93 added a comment to D93532: [ADT] Add resize_for_overwrite method to SmallVector..

Given that we own SmallVector and destroy_range is a no-op, is it undefined behaviour?

If the problem is the call to new (&*) T;, then can we add a function in SmallVectorTemplateCommon (or whatever it is that's specialized for PODs) called uninitialized_construct or something that's definitely a no-op?

If the call to new (&*) T; is tripping sanitisers up, I'm happy to keep that behaviour. After calling resize_for_overwrite it should be required that you write to any newly allocated items before you read them, Explicitly making it a no-op will hide that testing route.
For the record I can't seem to run gtest under msan, there seems to be a false-positive use-of-uninitialized-value occurring in basic_string::push_back, obviously not related to this change.

Sat, Dec 19, 2:59 AM · Restricted Project