User Details
- User Since
- Apr 17 2017, 9:32 PM (362 w, 2 d)
- Availability
- Available
May 3 2022
Mar 6 2020
Mar 5 2020
I don't want to cancel any parse jobs, I want to change whether and when they emit the preamble.
Feb 4 2020
Hmm, now I'm thinking: If we parse first without creating a preamble, we still have the Clang AST. Couldn't we create the preamble from the AST without reparsing and use that for subsequent parsing?
Feb 2 2020
To keep this short I didn't reply to the parts I completely agree with.
Jan 14 2020
We use the option only when SOURCE_DATE_EPOCH is set, and we assume that a recent enough tar is available then. Basically we say that if reproducible builds are desired, the proper tools should be available.
Jan 12 2020
Jan 9 2020
Nov 23 2019
Oct 9 2019
I've just stumbled into a case where I would need -stdlib=libc++, which this change would solve.
I still think this is the right change, but if there is no consensus I will implement @mwolff's suggestion. That would certainly be an improvement, but it doesn't really solve my problem. I'm currently working on Clang and have around 6 files open, which is really not a lot. I haven't changed any of them. The result:
Jul 20 2019
Overall I think this is a good idea, although we need to figure out some details. Could you either remove commented out flags or add a comment why we have them (commented out)?
I don't know what would be required to support multiple DUChains, but it seems plausible that they can be locked separately. In this case it would help to have the explicit argument, I guess. It could even be inevitable, when there is no single global DUChain anymore.
Jul 10 2019
Jul 8 2019
Exactly.
Jul 7 2019
That's a bold statement. ;)
Jul 4 2019
Jul 3 2019
I looked through all of the paths to the failed assertion discussed in that latter ticket and it appears that all but the invocation in PotentialBuddyCollector::accept is verified to have the reader lock held prior to invocation. There were two paths which needed a lock added given my change, so this change set should not cause any regression on bug #386901.
Feb 14 2019
You're right, it's a precompiled header file. I think it works very well for the intended purpose: quick reparsing on most user edits. The additional option that we use here was added in https://reviews.llvm.org/D15490 for pure code completion workloads. I don't think it's meant for us, even though we do completion as well: we also build an index, and have possibly many more files open.
I wouldn't want to give up on a consensus just yet. I think the Clang developers have actually chosen a pretty smart default, and this additional flag incurs costs that are hard to control for hardly any benefit.
Feb 13 2019
Feb 11 2019
It only happens on the very first edit though, and will only be noticeable if code completion is required immediately after starting to edit the file. All following parses and code completions can use the precompiled preamble then. I don't think that's too bad. It's also not unusual, many applications have something like a "warmup phase" where not everything is loaded yet.
Feb 6 2019
Feb 5 2019
Feb 3 2019
Feb 2 2019
The way I understand it, the proposals are slightly different. Maybe we can summarize them as follows:
Feb 1 2019
I'm fine with that, but would prefer never having it. Not all files that are opened will be edited, and the delay caused by creating the preamble after the first edit is probably negligible over the lifetime of a session. It would also allow us to just drop affected preambles when a header is edited — otherwise we might have to recreate all preambles that include the header.
Jan 31 2019
Jan 29 2019
It definitely does. Sometimes I just cancel it, but sometimes I need the entire index to understand some code. (“Find uses” is very useful for that.)
Preambles are there to speed up repeated reparsing. Most translation units won't be reparsed in a typical session. Since we pay for every preamble we create, I don't think we should create them unless we've some indication that a file needs to be parsed again. It's pretty safe to assume that a file needs to be reparsed again if we parse it for the second time. There is probably no better indicator, which is why this is the default.
Jan 26 2019
Wouldn't that be an opportunity to add the override specifier? The commit message could state "Fix function signature of ... to override".
Yes, I believe that's not going to work. Clang doesn't keep the preamble files open, but expects them to stay.
I've had a look at the Clang code. They actually go to some lengths to make sure all temporary files are deleted on a clean exit — they register all open temporary files and clean them up in an exit-time destructor. (See TemporaryFiles in lib/Frontend/PrecompiledPreamble.cpp.) However, this won't work in the case of a crash. They have some mechanisms for cleanup in the case of crashes, but I suspect that this works by setting a signal handler and is thus only available in the stand-alone executable. There are mechanisms to ensure cleanup in any event by removing an open file on POSIX compatibles (close will then automatically delete it), or FILE_FLAG_DELETE_ON_CLOSE on Windows. But that doesn't work for Clang, because they work with paths and don't pass file descriptors around.
Jan 25 2019
Jan 24 2019
Jan 23 2019
Jan 17 2019
Dec 10 2018
Nov 18 2018
Nov 14 2018
Oct 20 2018
Thanks for the quick review!
The signal handling and processing gets a +1 from me, but someone else should review the overall code structure.
Oct 19 2018
Oct 18 2018
Oct 17 2018
Oct 16 2018
You're right, my remarks apply to the existing code already.
Oct 15 2018
Oct 4 2018
Don't we know the exact version of Clang at compile-time when building the bundled version? I think this change is mainly interesting for the "unbundled" case when Clang can be updated independently. Or what am I missing?
Oct 3 2018
Oct 2 2018
@kossebau Are you Ok with this change? I'm not sure I'll get a review from the domain experts, but I'm fairly confident now that the changes are right.
Sep 27 2018
Attributes are not keywords — they're a (C++-specific) generic syntax that was introduced with C++11. There are numerous compiler-specific attributes, usually prefixed with the compiler name, and some standardized attributes. See cppreference.com for a list of those.
Given that languageOption also has Q_UNREACHABLE(), and both are always called together, any crash that could happen would have already happened before. The function is called from CompilerProvider::{defines,includes} and both catch the Other case before calling into the ICompiler implementation. Maybe we should document the constraint (type != Utils::Other) in the ICompiler interface, but we should do it in a separate commit.
Sep 26 2018
I didn't test it, but it looks fine to me.
So I think there are two questions to decide here:
- Do we want a -std= flag, does Clang even accept that?
- Should we just take the ObjC flags from the C flags, and ObjC++ flags from the C++ flags, or should they be set independently?
Sep 23 2018
After testing the GitHub provider and the QmlJS code completion I'm somewhat confident that these changes are correct, but I want to give the experts on that code a chance to weigh in before I merge this. Now onto the discussion.
Remove comment in plugins/ghprovider/ghproviderwidget.cpp, as the fallthrough is probably intended, and add break in plugins/qmljs/codecompletion/context.cpp, as it is probably not intended. Now the code completion doesn't suggest variables of the same type as the last parameter in functions calls, which seems about right.
Sep 22 2018
There are two suspicious fallthroughs, maybe someone with a better understanding than me can comment on that.
Sep 21 2018
You should probably also add a few lines to the custom-definesandincludes plugin.