- User Since
- Feb 11 2014, 3:42 PM (344 w, 4 d)
Thu, Sep 3
Wed, Sep 2
Tue, Sep 1
SourceRange cxloc::translateCharRangeToTokenRange(CXTranslationUnit TU, CXSourceRange R) is an expensive operation and I'm concerned it will be easy to start calling at places and introduce performance degradation.
Mon, Aug 31
We may have places in the code where SourceRange is used as a pair of locations, and those locations are character locations instead of token ones, so essentially the information of whether the range is token-based or character-based gets lost, and we get into trouble when passing such a SourceRange to APIs that assume token-based.
AFAIK SourceRange is supposed to always represent a token range (begin loc points to beginning of first token, end loc points to beginning of last token). For representing a character range, CharSourceRange should be used, though IMO its IsTokenRange member was a mistake, CharSourceRange should have only being used to represent half-open character-based range. This distinction is thankfully more clear on the Swift side.
Jul 14 2020
Jun 29 2020
Jun 19 2020
May 20 2020
Mar 31 2020
Mar 28 2020
I agree it makes sense to disable CLANG_ENABLE_ARCMT if CLANG_ENABLE_STATIC_ANALYZER is also disabled.
Mar 7 2020
Feb 11 2020
Dec 20 2019
Dec 16 2019
Dec 12 2019
Oct 29 2019
Oct 8 2019
Aug 28 2019
Aug 24 2019
Aug 23 2019
Are you sure the difference is not random noise? It's not clear to me under which conditions you would encounter the same region in NewParsedRegions within the same TU. I assume this would be the case if the same file is parsed multiple times, but I'd expect this to be uncommon within the same TU.
I'd recommend to add some counters of when NewParsedRegions.count(region) is true and when ParsedRegions.count(region) is true, and print them at the end.
Aug 13 2019
Aug 7 2019
Sorry, it's not clear to me what is the issue.
Jul 22 2019
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.