- User Since
- Dec 23 2019, 11:05 AM (39 w, 6 d)
Fri, Sep 25
Updated function doc
Fix potential assertion
Wed, Sep 16
So, I think either this patch needs to be landed after some tweaking to try and bring the compile size down, though as it only appears to be an issue with clang it can probably afford some regression on gcc. Or we explicitly state that small vector is not safe to self reference itself and add instrumentation to enforce that behaviour. Right now we are in a middle ground where there are some guarantees made but not enough.
I would hedge my bets that there are definitely some code clients(whether in tree or not) that are currently using SmallVector incorrectly and sometimes run into little bugs like this.
Also for the record I have just built with gcc and noticed the same kind of regressions on the text size so at least I can have a look into this myself
Tue, Sep 15
Please fix the test case first, can't call operator new(unsigned long, void*) with an argument of type void*
The other failures the pre merge bot detected can safely be disregarded
@nikic Would you be able to see what the delta with this is https://gist.github.com/njames93/f26f159f06bda9e7ed2270adb39d9b08, should apply on top of trunk. It has the most expensive part of the grow buffer defined outline to dissuade inlining, may reduce binary size and improve performance with gcc9.3
I'm still seeing a different picture with clang-10 as the host compiler, I'm only targetting x86 which explains why my binaries are so much smaller than yours.
orig-o3 54404996 this-03 54046228 orig-lto 69549814 this-lto 65981798
The most likely reason for a larger binary in GCC is in the original file, the calls to grow weren't inlined. Now, most of that code for using a new buffer is inlined. Clang may handle things a little differently.
Maybe I could define the swap buffer methods out of line to dissuade inlining.
Mon, Sep 14
Add tests for SmallVector::assign to ensure contents aren't copied excessive amounts of times.
Please fix the clang-format fail too.
Remove excessive this-> from GrowBuffer.
Use range-based loop instead of for_each.
SmallVector seems to use global namespace new for all its operations, so I thought I'd follow the convention there.
Format was provided by clang-format-11. the pre-merge bot is using 10 so I think I should ignore the format messages.
Move grow_size into SmallVectorBase
Fix for the case if anyone tries SmallVector.emplace_back(SmallVector[X]) being invalidated on growth.
Added missing else if inside SmallVector append methods.
Refactored GrowBufferBase to take the Size_T as the template arguments to reduce template instantiations.
Use global namespace new inside GrowBuffer.
Yeah thats whats happening here.
Sun, Sep 13
Address a few inlines
Undo SmallVectorTest fix
Sat, Sep 12
Thu, Sep 10
What build flags are you using. I tried a release build with thinlto. Also tried to move some of this code into the cpp file by type erasing the template out of GrowBufferBase.
For the record I'm testing on a Ryzen 2600X on ubuntu 20.04 and using clang-10 as the host compiler and linking with lld-10
trunk - size.text = 42483D6, size.binary = 6141438 this - size.text = 3EBD276, size.binary = 5DDD940 type-erase - size.text = 3EC0B26, size.binary = 5DE1290
Fix build error when appending iterators of types convertible to SmallVector::value_type
Add buffer to append(iterator, iterator) to handle SmallVec.append(SmallVec.begin(), SmallVec.end());
Rebased trunk and fixed up assign to use new buffer
Wed, Sep 9
Extend behaviour to more methods.
Harden test cases for SmallVector
For the clang tidy naming warnings, is it better to follow the warnings, or the current convention of the file?
Tue, Sep 8
Undo fix for self referencing insertion
Mon, Sep 7
The no context is easy to spot as phab says context not available. Its easy to find knowing that there is no mention of hungarian notation in the trunk version of IdentifierNamingCheck.cpp, yet there is mention of that in the before diff.
Did you upload this incorrectly again, context is missing and seems to be a relative diff from a previous version of this patch?
Sun, Aug 30
As Hungarian notation enforces prefixes as well as casing styles it would be wise to warn on cases where the style is Hungarian but a prefix has also been set.
Aug 28 2020
Can you make sure you upload diffs with full context (-U99999). Or using arcanist it will be done automatically.
Aug 27 2020
Aug 22 2020
Aug 20 2020
Aug 18 2020
Aug 13 2020
Aug 11 2020
Should a line be added to the release notes to explain this behaviour:
'Checks that specify files to include now support wrapping the include in angle brackets to create a system include'?
Is 'over-unscoped' really needed in the name, would just 'prefer-scoped-enum' be better, WDYT?
Aug 10 2020
Aug 8 2020
LG, pretty trivial fix. Side note running that check over the code base leads to a lot of warnings, however most of false positives, like using abbreviations in the comment or parameter.
Aug 7 2020
Aug 6 2020
Also with a name like compare op parenthesis. It sounds like this would consider == and !=
Aug 4 2020
I think if this new function was a non-member, it'd probably tidier - wouldn't need an rvalue and non-rvalue overload, as it could use std::forward on both arguments.
non-member could work I guess, and could be adapted for other cases, std::optional or pointers spring to mind.
Aug 3 2020
- Rebased trunk
- Cleaned up test cases
- Added support for specifying to include as system include.
Delete the rvalue reference version as it can create dangling references
If that's the case then I'd agree with that.
But I kind of gathered it wouldn't be the case considering how much the llvm::Optional API already differs from std::optional.
Probably doesn't need the enable_if, I'd have thought (could just let the implementation fail as usual - there aren't competing member functions, etc)
Fair enough point there.
the parameter should be U&& to use with std::forward, avoid unnecessary copies, etc.
I'm inclined to disagree with that point, this is mean't to be used with types that are cheap to construct and usually passed by value.
Probably don't need the call to std::forward though.
Remove call to forward as it serves no purpose for the use case
Aug 1 2020
Fix unpunctuated comment and simplify second check command.
- Address comments.
- Added test case for when this behaviour is disabled.
Jul 31 2020
Added Option GetConfigPerFile to control this behaviour.
Ensure the check is enabled in the header files configuration before using any configuration found.
Jul 30 2020
Fix build errors
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
LGTM, Thanks that bug was eating away at me for a good few days.
For the record X < Y < Z does have a mathematical meaning, Y is constrained between X and Z.
However in the context of C the expression isnt parsed like that.
If someone writes this they likely wanted (X < Y) && (Y < Z)
For this specific check as you pointed out we wouldn't want to make that assumption though there is a case for adding notes to silence the warning by wrapping one of the comparisons in parenthesis.
Jul 29 2020
Rebase from parent and address comments.
Revert to using warning for logging parsing errors
Missing call to getValue()