- User Since
- Feb 11 2014, 3:42 PM (287 w, 6 d)
Tue, Aug 13
Wed, Aug 7
Sorry, it's not clear to me what is the issue.
Mon, Jul 22
Jul 16 2019
May 23 2019
May 10 2019
Mar 6 2019
Mar 5 2019
Mar 1 2019
Feb 23 2019
Feb 8 2019
Dec 6 2018
Dec 5 2018
Nov 27 2018
Nov 25 2018
Thanks for reviewing Kristina & Aaron, much appreciated!
- Change #cmakedefine01 to #cmakedefine for consistency.
- Add comments to getLastAccessedTime()/ getLastModificationTime() APIs to make sure people are aware that time resolution can differ among file systems.
- Improve commit summary.
I think that should be documented on the public API part with a mention that the resolution is expected to differ depending on the file system and OS. WDYT?
That is a good idea, the way I consider file_status is that it provides a cross-platform way to get at the file properties. It provides times in nanoseconds but leaves the precision as implementation detail of the underlying file system, which makes sense, people should not expect anything more than that.
I'll add some comments.
I can't speak for the Windows side of things but what you are pointing out is a general question about applications taking into account that file properties may differ across platforms. This is orthogonal to file_status itself reporting file properties accurately.
If current file_status has 1 hour resolution on NTFS for access time, this patch is not going to make it any better or worse, nor affect behavior of applications that are querying files from NTFS.
I think this is the most important bit of information here. It should have been in the description of the differential.
If the current precision was 1ms (or even 1us), then yes, going all the way down to 1ns may not be important/worthwhile.
But 1 sec precision is clearly laughably not great, and should be improved.
Ah good point, I didn't consider that people are not broadly aware that current file_status has only 'up to second' precision.
On second look, if you are concerned with adding additional fields in basic_file_status, I can replace:
time_t fs_st_atime = 0; time_t fs_st_mtime = 0;
with uint64_t's which are adequate to include nanosecond precision, while these time_t's are 64bits as well but waste space in the structure.
if there's no consumer for such API (pending review) there is no point in adding it.
To make sure it's clear, this is not a new API. getLastModificationTime() already reports a a timepoint in nanoseconds, but it is always in second intervals. This patch allows it to include the nanoseconds that are part of the underlying file status.
The general changes to the support libraries are usually facilitated because of missing functionality that is required somewhere else and it being the most logical place to add it. If so please attach related revisions to this one (through "Edit Related Revisions") so it's more clear why this was added
There have been enhancements to file_status that added more info without the requirement you stated (attach related revisions to this one), like:
Nov 22 2018
Moved the toTimePoint conversion function to llvm/Support/Chrono.h so it can be broadly available.
Nov 21 2018
Nov 15 2018
Aug 28 2018
Aug 1 2018
Jul 19 2018
Good enough, thanks for looking into this!
CXIndexDataConsumer.cpp uses ASTNode.OrigD, that code is exercised when you run c-index-test -index-file <...>. I'd recommend to at least check if that command would produce a different output for the test case that you detected originally.
Jul 18 2018
Is it possible to add a regression test case ? I assume this is fixing some issue, we should make sure we don't regress again.
Jun 1 2018
We could leave disableSourceFileDiagnostics off until someone finds a use case for it.
May 31 2018
Thanks for the explanation. Please do add documentation comments for the new method so people using ASTUnit in their own code have an idea when and why they would need to call this. Something like "if you intend to emit additional diagnostics after the ASTUnit is created [...]".
Also consider making the naming more clear to match the intended purpose, like enableEmittingAdditionalDiagnostics() or something similar.
May 30 2018
It's not clear to me what kind of issue you are addressing, for example is the unit test hitting an assertion hit without your changes ? Or is there something else ?
Also it would be good to add some documentation comments to clarify what's the use case and when ASTUnit::beginSourceFile() should be getting called.
Mar 16 2018
That would be good. How would one go about asking Clang to generate this extra information? Would a Clang Plugin be suitable for this?
That's an interesting idea that we could explore, but I don't have much experience with that mechanism to comment on.
Mar 14 2018
Jan 26 2018
Jan 10 2018
Jan 8 2018
Ah, sorry I mislead you. To test this try using c-index-test -index-file /path/to/file, see other examples in test/Index, e.g. test/Index/index-file.cpp
Could you add a test case for this change ? See examples in test/Index/Core.
Also for the test case code: template <template <typename> class Actor = actor>, is the actor reference in this code reported ?
See the test examples on how to print out and test how the source symbols are indexed.
Dec 23 2017
Dec 21 2017
Nice one! One minor change I'd suggest is to change SymbolProperty to enum class SymbolProperty : SymbolPropertySet, so that if it needs to increase we only change the type in one place.
Dec 7 2017
@malaperle, to clarify we are not suggesting that you write your own parser, the suggestion is to use clang in 'fast-scan' mode to get the structure of the declarations of a single file, see CXTranslationUnit_SingleFileParse (along with enabling skipping of bodies). We have found clang is super fast when you only try to get the structure of a file like this. We can make convenient APIs to provide the syntactic structure of declarations based on their location.
Nov 16 2017
To be honest, I want this functionality to get in as much as you do, and I'm more than happy to prioritize the code review for it :) But the current patch size makes the reviewing really hard (e.g. I would never have caught the BLOCK issues hadn't I tried running the original patch myself). I'm not sure if it's really a common practice to check in a big chunk of code without careful code review and leave potential improvements as followups. I'm sure @klimek would have thoughts about this.
Nov 10 2017
Nov 1 2017
Oct 23 2017
Oct 17 2017
FYI, I had added -target in this test in r296263 but it was still failing for some reason in some bots, that's why I then switched to triple via r296265.
I'm not sure if it's going work now or not for all bots, but it does, that is definitely better.
Jun 27 2017
I'd prefer to avoid including whitespace-only changes (there are a couple of lines in the diff with only whitespace change), otherwise LGTM!
Jun 26 2017
FullSourceLoc could be useful, it wraps the SourceManager that the SourceLocation come from.
Comparing SourceLocations from different translation units is not meaningful and my concern is that treating source locations like this can very easily lead to errors where by mistake the code is resolving a SourceLocation with the wrong translation unit and not the translation unit that it came from.
Jun 19 2017
Update doc-comment for Preprocessor::EvaluateDirectiveExpression
Provide doc-comments for struct DirectiveEvalResult.
There is a bit of misunderstanding on what this patch is doing, the patch is not about answering "any symbol is defined". See my cfe-dev email (http://lists.llvm.org/pipermail/cfe-dev/2017-June/054254.html) that provides some more context.
The patch is doing these 2 things:
- Keep track of whether a preprocessor directive condition used a symbol that was not resolved at all
- If above is true then make the preprocessor parse all directive blocks, not arbitrary choose one of them.
There is no other patch for using this preprocessor option. Any related improvements down the road will be about general improvements for error recovery, for example things like this:
- Introduce an UnresolvedTypename type instead of changing unresolved types to int.
- For @interace A : B, don't completely drop B from the super-class list of A if it is unresolved.
These kind of improvements are not conditional.
Jun 16 2017
Improving doc comment.
Enhanced doc-comment for the preprocessor option, and fixed indentation on a couple of calls.
Hey Vladimir, what you are proposing is orthogonal to this patch. You are proposing for "the client to provide the value for an undefined identifier", and the patch is about the client not knowing what the value should be so it fallbacks to parsing all tokens to get the max amount of info. Note that both of the techniques can be combined well, if the client provides the value, the preprocessor will take it into account, otherwise if it is stays unresolved it will fallback to lexing all tokens.
But what you are proposing is not a replacement for what the patch is doing.
Jun 15 2017
Jun 2 2017
I retract my comment, I see that getMainExecutable on OSX calls realpath as well, so it's good to use realpath in this code to make sure they are in-sync with how the compiler will determine the path.
Getting the real path is notoriously slow (at least it's horrible in OSX, not sure about linux). Since this is about dropping the '/../' part, could we do some simple canonicalization of removing the dots ? Not sure if there is an existing function that does that.
May 19 2017
Also used the new Decl:: function in a place in libclang/CIndex.cpp and committed in r303484
May 18 2017
I recommend to add a Decl::getExternalSourceSymbolAttr() to reuse this and avoid code duplication.
Apr 21 2017
Committed in pieces with r298392, r300948, r300949.
Mar 17 2017
Committed in r298170.
Mar 16 2017
Committed in r297972.
Feb 25 2017
Committed in r296282 with a couple of changes:
- Made it so that parameter definitions have a 'child' relation with their function/method (instead of 'containedBy'). This allows looking up parameters of a function by looking up their children.
- Given that parameters show up in parent-child hierarchies, I think it is a bit better to make them to a separate 'SymbolKind' instead of a sub-kind. This preserves the property that 'variable' symbol kind does not ever enter a parent-child hierarchy.
Feb 23 2017
Feb 16 2017
Committed in r295416, thanks!
Dec 16 2016
Committed via r289995, thanks!
Dec 1 2016
When I try just the test case (without the other changes), it doesn't fail when using r288438; so I'm not sure whether the test is actually testing the regression.
LGTM, thanks! Committed in r288438.
Nov 15 2016
LGTM, thanks! Committed in r287024.
Nov 9 2016
Committed in r286421, thank you!
The fix is correct, the comment for CXSourceRange says that it "Identifies a half-open character range in the source code".
But you don't need the other changes to test this. Here's an example:
Oct 28 2016