Page MenuHomePhabricator

hintonda (Don Hinton)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 18 2013, 5:29 PM (309 w, 2 d)

Recent Activity

Today

hintonda updated the diff for D62353: [cmake] Add new LLVM_CACHE_VARIABLES variable to contains all variables passed to cmake on the commandline or in cache files..
  • Fix typo.
Thu, May 23, 4:26 PM · Restricted Project
hintonda added inline comments to D62353: [cmake] Add new LLVM_CACHE_VARIABLES variable to contains all variables passed to cmake on the commandline or in cache files..
Thu, May 23, 4:22 PM · Restricted Project
hintonda created D62353: [cmake] Add new LLVM_CACHE_VARIABLES variable to contains all variables passed to cmake on the commandline or in cache files..
Thu, May 23, 4:19 PM · Restricted Project
hintonda created D62343: [cmake] Remove old unused version of FindZ3.cmake from clang [NFC].
Thu, May 23, 1:53 PM · Restricted Project
hintonda updated the diff for D62105: [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..
  • Removed unneeded public method from Option.
Thu, May 23, 1:03 PM · Restricted Project
hintonda added inline comments to D62105: [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..
Thu, May 23, 11:04 AM · Restricted Project
hintonda committed rG0857a4ec20db: [cmake] When getting Ninja version, don't include CMakeNinjaFindMake which… (authored by hintonda).
[cmake] When getting Ninja version, don't include CMakeNinjaFindMake which…
Thu, May 23, 8:04 AM

Yesterday

hintonda added inline comments to D62264: [CommandLine] WIP: Add new debug_opt that reduces down to size of DataType in Release builds..
Wed, May 22, 3:41 PM · Restricted Project
hintonda updated the diff for D62264: [CommandLine] WIP: Add new debug_opt that reduces down to size of DataType in Release builds..
  • Added cl::debug_opt_storage with specializations.
Wed, May 22, 2:56 PM · Restricted Project
hintonda added inline comments to D62264: [CommandLine] WIP: Add new debug_opt that reduces down to size of DataType in Release builds..
Wed, May 22, 11:20 AM · Restricted Project
hintonda created D62264: [CommandLine] WIP: Add new debug_opt that reduces down to size of DataType in Release builds..
Wed, May 22, 11:17 AM · Restricted Project

Tue, May 21

hintonda committed rG24d27689bc9c: [clang-tidy] remove default header-filter for run-clang-tidy (authored by hintonda).
[clang-tidy] remove default header-filter for run-clang-tidy
Tue, May 21, 6:00 PM
hintonda committed rG120a6f09bb95: [Docs] Increase Doxygen cache size (authored by hintonda).
[Docs] Increase Doxygen cache size
Tue, May 21, 5:55 PM
hintonda committed rGb61f2b6c8775: [cmake] Don't use VERSION_GREATER_EQUAL in cmake versions prior to 3.72. (authored by hintonda).
[cmake] Don't use VERSION_GREATER_EQUAL in cmake versions prior to 3.72.
Tue, May 21, 12:27 PM
hintonda committed rG76e5a1d3c3b1: [cmake] Try to make cmake happy and fix bots. (authored by hintonda).
[cmake] Try to make cmake happy and fix bots.
Tue, May 21, 11:50 AM
hintonda committed rGc1b6b9a17776: [cmake] Bug in r361281: make include optional and fix typo which might make a… (authored by hintonda).
[cmake] Bug in r361281: make include optional and fix typo which might make a…
Tue, May 21, 11:13 AM
hintonda committed rGbd467cfe4bcd: [cmake] Add custom command to touch archives on Darwin so ninja won't rebuild… (authored by hintonda).
[cmake] Add custom command to touch archives on Darwin so ninja won't rebuild…
Tue, May 21, 10:55 AM
hintonda updated the diff for D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..
  • Use CMAKE_HOST_APPLE when checking host system.
Tue, May 21, 10:46 AM · Restricted Project
hintonda updated the diff for D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..
  • Remove tab.
Tue, May 21, 9:52 AM · Restricted Project
hintonda added a comment to D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..

It is silly that CMake doesn't expose the ninja version, but it is easy enough to get it. It should be something like:

if(CMAKE_GENERATOR STREQUAL "Ninja")
  execute_process(COMMAND ${RunCMake_MAKE_PROGRAM} --version
      OUTPUT_VARIABLE ninja_version OUTPUT_STRIP_TRAILING_WHITESPACE)
  if(ninja_version VERSION_GREATER 1.8.2)
    set(ninja_greater_1_8_2 On)
  endif()
endif()

I know this is a bunch of seemingly odd boiler plate, but since we're working around tools bugs in non-LLVM tools which will hopefully someday be fixed, I'd like to restrict this fix to only places where we know the bug is present.

Tue, May 21, 9:45 AM · Restricted Project
hintonda updated the diff for D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..
  • Don't put new variable in the cache.
Tue, May 21, 9:45 AM · Restricted Project
hintonda updated the diff for D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..
  • Remove unneeded else branch.
Tue, May 21, 9:24 AM · Restricted Project
hintonda updated the diff for D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..
  • Get Ninja version and add LLVM_TOUCH_STATIC_LIBRARIES.
Tue, May 21, 8:55 AM · Restricted Project
hintonda updated the diff for D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..
  • Restrict to Darwin > 15.6.0. Ninja version not available, so will add code to get that and hide all of this under a new variable in the next version of this patch.
Tue, May 21, 8:17 AM · Restricted Project
hintonda accepted D62174: [Analysis] Link library dependencies to Analysis plugins.

LGTM. Build and check-llvm were both clean on my Mac for static build. Thanks!

Tue, May 21, 7:39 AM · Restricted Project, Restricted Project
hintonda abandoned D62154: [cmake] Revert part of r360991 "[Analysis] Only run plugins tests if plugins are actually enabled" that unilaterally disabled plugins.

D62174 is a better solution.

Tue, May 21, 7:22 AM · Restricted Project

Mon, May 20

hintonda added a comment to D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..

I have a long story about this issue... Ask me about it sometime :).

We should get @thakis to weigh in here, but Ninja 1.8.2 works on Darwin because it uses the older filesystem APIs and ignores the high-precision timestamps. If we take this patch we should restrict it more. I suspect that libtool on Linux probably behaves correctly. Darwin's binutils fork is literally decades old.

My guess at restricting the patch would be only on Darwin (which is the only place we use libtool), only if the Ninja version > 1.8.2, and Darwin version > 15.6.0 (I think 16.0.0 was when APFS went in which causes this issue).

Mon, May 20, 10:58 PM · Restricted Project
hintonda added a comment to D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..

@thakis Didn't we patch Ninja for this?

Here's what I'm running on my Mac:

local:/Users/dhinton/projects/llvm_project/monorepo/llvm-project $ ninja --version
1.9.0
local:/Users/dhinton/projects/llvm_project/monorepo/llvm-project $ cmake --version
cmake version 3.14.0

Mon, May 20, 10:15 PM · Restricted Project
hintonda added a comment to D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..

@thakis Didn't we patch Ninja for this?

Mon, May 20, 9:59 PM · Restricted Project
hintonda created D62172: [cmake] Add custom command to touch archives so ninja won't rebuild them..
Mon, May 20, 9:22 PM · Restricted Project
hintonda added a comment to D62171: build: Enable CMake Policy 0077.

I think people have learned to overcome the OLD behavior by including CACHE and type in set commands in cache files, but this makes it easier. However, if you want to rerun cmake and change existing values, you'll still need to include CACHE, type, and FORCE, is that right? At least that's how I've been handling it locally. Any advice would be appreciated.

Mon, May 20, 9:13 PM · Restricted Project
hintonda added a comment to D62050: [Analysis] Only run plugins tests if plugins are actually enabled.

This also breaks "ninja check-cfi-and-supported" on clean build (run cmake in empty directory)

I've fixed this with DLLVM_BUILD_LLVM_DYLIB=ON but not sure if this is the right approach long-term.

Mon, May 20, 7:45 PM · Restricted Project, Restricted Project
hintonda added a comment to D61697: [lit] Disable test on darwin when building shared libs..

ping...

Mon, May 20, 4:54 PM · Restricted Project, Restricted Project
hintonda added a comment to D62157: Remove explicit header-filter in run_clang_tidy.py.

@hintonda Aha so it is :)

Mon, May 20, 4:18 PM · Restricted Project
hintonda accepted D61747: [clang-tidy] remove default header-filter for run-clang-tidy.

LGTM. If no further comments, I'll commit it for you tomorrow.

Mon, May 20, 3:54 PM · Restricted Project, Restricted Project
hintonda added a comment to D62157: Remove explicit header-filter in run_clang_tidy.py.

Great idea.

Mon, May 20, 3:50 PM · Restricted Project
hintonda created D62154: [cmake] Revert part of r360991 "[Analysis] Only run plugins tests if plugins are actually enabled" that unilaterally disabled plugins.
Mon, May 20, 10:10 AM · Restricted Project
hintonda added inline comments to D62050: [Analysis] Only run plugins tests if plugins are actually enabled.
Mon, May 20, 9:54 AM · Restricted Project, Restricted Project
hintonda added inline comments to D62050: [Analysis] Only run plugins tests if plugins are actually enabled.
Mon, May 20, 9:38 AM · Restricted Project, Restricted Project
hintonda updated the diff for D61554: [llvm] Add CloneModuleInto to clone a Module into another one, a la assignment.

Rebase only...

Mon, May 20, 8:04 AM · Restricted Project
hintonda accepted D62138: [Docs] Increase Doxygen cache size.

I'll give it a day for further comments, and barring any, commit it for you tomorrow.

Mon, May 20, 7:34 AM · Restricted Project, Restricted Project

Sun, May 19

hintonda retitled D62105: [CommandLine] Remove OptionCategory and SubCommand caches from the Option class. from [CommandLine] Remove SubCommands SmallPtrSet from the Option class. to [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..
Sun, May 19, 1:31 PM · Restricted Project
hintonda updated the summary of D62105: [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..
Sun, May 19, 1:31 PM · Restricted Project
hintonda updated the diff for D62105: [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..
  • Remove Categories cache as well. This change reduces the size of the Option class down to 64 bytes.
Sun, May 19, 1:24 PM · Restricted Project
hintonda updated the diff for D62105: [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..
  • Add comments, fix auto usage, and minor refactoring.
Sun, May 19, 10:51 AM · Restricted Project

Sat, May 18

hintonda retitled D62105: [CommandLine] Remove OptionCategory and SubCommand caches from the Option class. from [CommandLine} Remove SubCommands SmallPtrSet from the Option class. to [CommandLine] Remove SubCommands SmallPtrSet from the Option class..
Sat, May 18, 10:10 PM · Restricted Project
hintonda added a comment to D62105: [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..

The Option class started at 184 bytes, this change, along with D62091, gets it down to 88 types, and reduces bin/opt by over 90k.

Sat, May 18, 10:10 PM · Restricted Project
hintonda created D62105: [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..
Sat, May 18, 10:01 PM · Restricted Project
hintonda committed rG4b105f53082b: [CommandLine] Reduce size of Option class (authored by hintonda).
[CommandLine] Reduce size of Option class
Sat, May 18, 1:44 PM
hintonda updated the diff for D62091: [CommandLine] Reduce size of Option class.
  • Reduce initial vector/set size to the expected value.
Sat, May 18, 12:43 PM · Restricted Project
hintonda updated the summary of D62091: [CommandLine] Reduce size of Option class.
Sat, May 18, 12:38 PM · Restricted Project
hintonda retitled D62091: [CommandLine] Reduce size of Option class from [CommandLine] Reduce size of Option class by moving more members into bit field to [CommandLine] Reduce size of Option class.
Sat, May 18, 12:38 PM · Restricted Project
hintonda added inline comments to D62091: [CommandLine] Reduce size of Option class.
Sat, May 18, 12:19 PM · Restricted Project
hintonda added inline comments to D62091: [CommandLine] Reduce size of Option class.
Sat, May 18, 11:58 AM · Restricted Project
hintonda added a comment to D62091: [CommandLine] Reduce size of Option class.

This is pretty cool. Since these are usually allocated as globals, this is saving pages of dirtied memory.

One implementation thought. What if we group the 16-bit fields together and make them uint16_t then use a uint16_t as the bitfield type for the remaining 15 bits. The performance of the 16-bit fields should be better if they are aligned (not that cl::opt perf really matters).

Sat, May 18, 8:19 AM · Restricted Project

Fri, May 17

hintonda updated the diff for D62091: [CommandLine] Reduce size of Option class.
  • Remove unneeded bit fields.
Fri, May 17, 8:21 PM · Restricted Project
hintonda updated the diff for D62091: [CommandLine] Reduce size of Option class.
  • Change type to multiple uint16_t's and reorder members.
Fri, May 17, 7:53 PM · Restricted Project
hintonda created D62091: [CommandLine] Reduce size of Option class.
Fri, May 17, 5:39 PM · Restricted Project
hintonda added a comment to D61956: [CMake] Add first CMake cache files.

Btw, options on the command line always override what's in the cache. Has nothing to do with FORCE. All FORCE does is make sure the set command actually changes an existing cache value. So it's an ordering issue. If the -D comes before the -C then using FORCE would override, but if the -C comes before the -D, the -D always overrides.

Fri, May 17, 3:03 PM · Restricted Project, Restricted Project
hintonda requested changes to D61870: Make cl::HideUnrelatedOptionsless error-prone.

Btw, TopLevelSubCommand can't actually see every option, just those not specifically assigned to another SubCommand. Typically, that only happens in applications, so this should work okay. However, it would probably be safer to iterate over all SubCommands instead of just looking TopLevelSubCommand. Though I'm not sure that really matters.

Fri, May 17, 2:30 PM · Restricted Project
hintonda added a comment to D61870: Make cl::HideUnrelatedOptionsless error-prone.

@hintonda / @beanz : I've been thinking about this, and I'm now convinced that this review is probably not the good approach. What we need is a good way to track which options are actually important for a given binary, correctly set the categories of each option and *reallyhide* the others. So I'll pause this review and start working on sanitizing the output of several tools until I get a better understanding of the actual API needs.

I created an example application yesterday to test this stuff out, and was just about to add a similar comment.

The problem with this API -- not your changes, but the API in general -- is that the HiddenFlag attribute is per cl::Option, not per cl::SubCommand. So, if you hide an option, it's hidden everywhere. However, finding the options you want to hide can be tricky. Only the special cl::SubCommand, cl::TopLevelSubCommand, which is the default, can see all registered options, so passing it is the only way to truly hide options not associated with the cl::Categorys of interest. Any other combination of calls to cl::HideUnrelatedOptions with various cl::SubCommands is unnecessary and error-prone.

Therefore, I recommend removing the SubCommand parameter from this API and always using cl::TopLevelSubCommand.

Fri, May 17, 9:55 AM · Restricted Project
hintonda added a comment to D61870: Make cl::HideUnrelatedOptionsless error-prone.

@hintonda / @beanz : I've been thinking about this, and I'm now convinced that this review is probably not the good approach. What we need is a good way to track which options are actually important for a given binary, correctly set the categories of each option and *reallyhide* the others. So I'll pause this review and start working on sanitizing the output of several tools until I get a better understanding of the actual API needs.

Fri, May 17, 9:41 AM · Restricted Project

Thu, May 16

hintonda added a comment to D61747: [clang-tidy] remove default header-filter for run-clang-tidy.

After thinking about it, I agree with this change -- there's no way to provide an appropriate default at this level.

Thu, May 16, 6:59 PM · Restricted Project, Restricted Project
hintonda added a reviewer for D61747: [clang-tidy] remove default header-filter for run-clang-tidy: hintonda.
Thu, May 16, 6:59 PM · Restricted Project, Restricted Project
hintonda committed rG8249a8889dbe: [CommandLine] Don't allow duplicate categories. (authored by hintonda).
[CommandLine] Don't allow duplicate categories.
Thu, May 16, 9:23 AM
hintonda added a comment to D61756: Add a __FILE_NAME__ macro..

Reverted in rL360842 as Windows bots were failing.

I suspect the MSVCCompat case may need to be handled differently depending on the host OS similar to PPDirectives.cpp:

  if (LangOpts.MSVCCompat) {
    NormalizedPath = Filename.str();
#ifndef _WIN32
    llvm::sys::path::native(NormalizedPath);
#endif
  }

Reopening this for now, will try to get a local Windows builder set up and work out what the problem is before relanding.

Thu, May 16, 8:52 AM · Restricted Project

Wed, May 15

hintonda updated the diff for D61972: [CommandLine] Don't allow duplicate categories..
  • Remove file erroneously included in last upload.
Wed, May 15, 8:42 PM · Restricted Project
hintonda updated the diff for D61972: [CommandLine] Don't allow duplicate categories..
  • Used find instead of find_if.
Wed, May 15, 8:25 PM · Restricted Project
hintonda added inline comments to D61972: [CommandLine] Don't allow duplicate categories..
Wed, May 15, 8:20 PM · Restricted Project
hintonda updated the summary of D61972: [CommandLine] Don't allow duplicate categories..
Wed, May 15, 6:40 PM · Restricted Project
hintonda created D61972: [CommandLine] Don't allow duplicate categories..
Wed, May 15, 6:37 PM · Restricted Project
hintonda added a comment to D61870: Make cl::HideUnrelatedOptionsless error-prone.

The basic --help-hidden output includes a lot of additional general options, though I haven't investigated where they come from. However, the --help-hidden output for a specific SubCommand does not include those options, so I'm not sure this is a problem (I'll investigate exactly how it all works and report back). I'm temped to think SubCommands aren't really needed in this API, since only the Options for the SubCommand are printed anyway. Again, I'll investigate and report back.

Wed, May 15, 3:31 PM · Restricted Project
hintonda added a comment to D61870: Make cl::HideUnrelatedOptionsless error-prone.

@beanz
As pointed out by @hintonda, current default argument is *never* explicitly used in the codebase.

Yea, but that's going to change as soon as you start calling this in more places. Have you looked at how this might apply to a tool that has sub-commands? My concern is that the ordering of default parameters in functions that have more than one really needs to be in the order of decreasing likelihood of whether or not it will be overridden. By my count we have 2 tools with subcommands that account for 16 SubCommand, and you have one example of overriding the enum. At the moment it seems to me like overriding the SubCommand may be the more common case.

Wed, May 15, 2:05 PM · Restricted Project
hintonda committed rG4c50e64fc5cf: [clang-tidy] Recommit r360785 "modernize-loop-convert: impl const cast iter"… (authored by hintonda).
[clang-tidy] Recommit r360785 "modernize-loop-convert: impl const cast iter"…
Wed, May 15, 10:46 AM
hintonda committed rG4ecb581188ff: Revert [clang-tidy] modernize-loop-convert: impl const cast iter (authored by hintonda).
Revert [clang-tidy] modernize-loop-convert: impl const cast iter
Wed, May 15, 10:35 AM
hintonda added a comment to D61747: [clang-tidy] remove default header-filter for run-clang-tidy.

I've also had issues with this behavior. It's a bad default, as the PR notes, but is there a way to provide a better one and still respect the config file?

Wed, May 15, 10:27 AM · Restricted Project, Restricted Project
hintonda committed rG42d28be802fe: [clang-tidy] modernize-loop-convert: impl const cast iter (authored by hintonda).
[clang-tidy] modernize-loop-convert: impl const cast iter
Wed, May 15, 9:59 AM
hintonda accepted D61870: Make cl::HideUnrelatedOptionsless error-prone.

Just a couple nits, otherwise looks great.

Wed, May 15, 9:09 AM · Restricted Project

Tue, May 14

hintonda added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

I'll be happy to commit for you, but will give it till tomorrow just to make sure no one else has any additional comments.

Tue, May 14, 12:03 PM · Restricted Project, Restricted Project
hintonda updated subscribers of D61870: Make cl::HideUnrelatedOptionsless error-prone.
Tue, May 14, 10:43 AM · Restricted Project
hintonda added a comment to D61870: Make cl::HideUnrelatedOptionsless error-prone.

Just a few nits, otherwise looks pretty good.

Tue, May 14, 10:17 AM · Restricted Project

Mon, May 13

hintonda added a comment to D61870: Make cl::HideUnrelatedOptionsless error-prone.

It was added in D7100, and I think the rational was to hide implementation details.

@hintonda: What about an optional `bool ReallyHide=true` paramer so that the same function could be use to really hide or just hide?

Mon, May 13, 10:42 PM · Restricted Project
hintonda added a comment to D61870: Make cl::HideUnrelatedOptionsless error-prone.

Thanks! I do like the llvm-cat -help changes, and this does make sense in that context.
@hintonda can you comment on why the lhs of the diff is the way it is, why these were cl::ReallyHidden?

It was added in D7100, and I think the rational was to hide implementation details.

Mon, May 13, 2:56 PM · Restricted Project
hintonda accepted D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

LGTM, but you may want to give @alexfh or @aaron.ballman a day to comment before you commit.

Mon, May 13, 2:25 PM · Restricted Project, Restricted Project
hintonda added a comment to D61870: Make cl::HideUnrelatedOptionsless error-prone.

Thanks! I do like the llvm-cat -help changes, and this does make sense in that context.
@hintonda can you comment on why the lhs of the diff is the way it is, why these were cl::ReallyHidden?

Mon, May 13, 2:16 PM · Restricted Project
hintonda requested changes to D61870: Make cl::HideUnrelatedOptionsless error-prone.

This is a great idea, but I'd prefer it not unilaterally change current behavior.

Mon, May 13, 2:11 PM · Restricted Project

Sun, May 12

hintonda added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.
Sun, May 12, 3:18 PM · Restricted Project, Restricted Project
hintonda added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

I pulled down you patch, compiled and ran it. Once I fixed the two problems I mentioned, it ran clean, e.g.:

Sun, May 12, 3:12 PM · Restricted Project, Restricted Project
hintonda added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

When I asked for a test above, I understood you to say you couldn't provide one, but If I misunderstood, by all means, please add the test.

Please do note that i have provided a testcase (godbolt link) in my very first comment, and quoted that line when replying the previous time.
(Granted, that loop is not in a correct form for openmp, but the point being, the current check does not diagnose it either)

Sun, May 12, 1:43 PM · Restricted Project, Restricted Project
hintonda added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),

As a general rule, don't add anything that doesn't include a test.

Since this "false positive" is apparently untestable,

How so?

Sun, May 12, 1:21 PM · Restricted Project, Restricted Project
hintonda added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Could you suggest a simple test case that could be added to the test? That way, instead of just removing the if else block, @torbjoernk could try to handle it. Or perhaps exclude it from the match altogether.

As i said, i don't see how this can be avoided in general.

I have to admit that I have very little experience with OpenMP and haven't thought of this at all. Thank you very much for bringing this up.

Would it help to extend the exclusion AST matcher for iterator-based loops by an exclusion for loops with an ancestor of ompExecutableDirective?:

return forStmt(
             unless(anyOf(isInTemplateInstantiation(),
                          hasAncestor(ompExecutableDirective()))),
Sun, May 12, 1:10 PM · Restricted Project, Restricted Project
hintonda added a comment to D61831: [cmake] Do not generate benchmark targets by default.

Hi @hintonda!

Could you please elaborate a bit on why you would like to propose this change? I couldn't understand the argument here:

Since the benchmark subproject doesn't depend on llvm, there's not reason to generate its targets by default, i.e., there are no dependencies and changes to llvm can't break it.

Sun, May 12, 1:05 PM · Restricted Project

Sat, May 11

hintonda created D61831: [cmake] Do not generate benchmark targets by default.
Sat, May 11, 7:15 PM · Restricted Project
hintonda committed rG0303e8a3fd88: [CommandLine] Add long option flag for cl::ParseCommandLineOptions . Part 5 of 5 (authored by hintonda).
[CommandLine] Add long option flag for cl::ParseCommandLineOptions . Part 5 of 5
Sat, May 11, 1:28 PM
hintonda added a comment to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.

This will now trigger on https://godbolt.org/z/9oFMcB right?
Just want to point out that this will then have "false-positives" when that loop
is an OpenMP for loop, since range-for loop is not available until OpenMP 5.

I don't think this false-positive can be avoided though, if building without
-fopenmp there won't be anything about OpenMP in AST,
and thus no way to detect this case..

Sat, May 11, 1:00 PM · Restricted Project, Restricted Project
hintonda added inline comments to D61827: [clang-tidy] modernize-loop-convert: impl const cast iter.
Sat, May 11, 12:27 PM · Restricted Project, Restricted Project

Fri, May 10

hintonda updated the diff for D61294: [CommandLine] Add long option flag for cl::ParseCommandLineOptions . Part 5 of 5.
  • Address comments.
Fri, May 10, 8:30 PM · Restricted Project
hintonda added inline comments to D61294: [CommandLine] Add long option flag for cl::ParseCommandLineOptions . Part 5 of 5.
Fri, May 10, 8:26 PM · Restricted Project
hintonda added a reviewer for D61697: [lit] Disable test on darwin when building shared libs.: sammccall.
Fri, May 10, 12:42 PM · Restricted Project, Restricted Project
hintonda updated the diff for D61660: [cmake] Make google benchmark project handle libraries properly when built in-tree.
  • Fix comment.
Fri, May 10, 12:32 PM · Restricted Project