Replace idgen with CTFE implementation

[jacob-carlborg]

Since id.h is not necessary anymore it's just as easy to implement the same functionality using CTFE. This will also simplify the build process because one extra build step is removed.

extern(C++) is removed as well because it will only be called from within D code.

This is required for #6771.

[dnadlinger]

id.h is still used by LDC.

[UplinkCoder]

@jacob-carlborg I am sorry to dismiss your work but honestly, this does not add significant value.
(But introduces breakage)

[wilzbach]

id.h is still used by LDC

Do you need this for the C++ backend? Or would this just mean that you need to upgrade your code base?

this does not add significant value.

I think this is great work as this hugely simplifies the build process and building DMD with dub (or any other build tool for that matter)!
@UplinkCoder: don't forget that this is a requirement for #6771.

[dnadlinger]

Do you need this for the C++ backend? Or would this just mean that you need to upgrade your code base?

Upgrade our codebase to what exactly? It is used by the C++ glue code to match some magic identifiers.

Edit: I suppose we could manually maintain a pair of .h/.cpp files with the constants we need somewhere.

[dnadlinger]

Even better than all this: Replace idgen completely by an id!"foo" template that initialises a member variable from IdPool using a static constructor.

Something like:

template id(string name) {
  __gshared Identifier id; // __gshared as the frontend is single-threaded anyway.
  shared static this() {
    id = Identifier.idPool(name);
  }
}

and replace Id.foo by id!"foo".

I'd rather not replace a crutch by another crutch.

[UplinkCoder]

I don't see why we'd have to get rid of idgen.
It works.

[WalterBright]

this is great work as this hugely simplifies the build process and building DMD with dub (or any other build tool for that matter)!

Even back in the 80s, IDEs and magic modern makefile replacements never could handle subprojects. This seems to remain a problem.

[WalterBright]

I suppose we could manually maintain a pair of .h/.cpp files with the constants we need somewhere.

That pretty much kills this PR. I guarantee that manually maintaining those files will be overlooked and then some poor sap will expend hours trying to figure out the breakage. The whole point of idgen was to eliminate those bugs, even when I was the only one who ever touched the code.

Until id.h is no longer needed, I'm closing this PR.

[yebblies]

@klickverbot How many uses of Id are there in the LDC glue? How many are used frequently enough that replacing with idPool would be an issue?

[dnadlinger]

I guarantee that manually maintaining those files will be overlooked and then some poor sap will expend hours trying to figure out the breakage.

But isn't Id only a cache for stringtable lookups anyway? I'd agree with your argument if there was the danger of, say, an enum member order getting out of sync. In this case, however, there don't seem to be any issues like that if we just maintain our own list of cached lookups (or directly use idPool). The actual identifier for a certain Id.<…> member being changed could indeed lead to an issue, but that shouldn't really happen.

[dnadlinger]

@yebblies:

$ grep -rI -e 'Id[(::).]' driver ir gen
driver/codegenerator.d:  Identifier id = Id.entrypoint;
gen/abi.cpp:  return (id == Id::__c_long) || (id == Id::__c_ulong) ||
gen/abi.cpp:         (id == Id::__c_long_double);
gen/asm-x86.h:      opIdent = Id::___in;
gen/asm-x86.h:      opIdent = Id::__int;
gen/asm-x86.h:      opIdent = Id::___out;
gen/asm-x86.h:           ((IdentifierExp *)exp)->ident == Id::__LOCAL_SIZE;
gen/asm-x86.h:           ((IdentifierExp *)exp)->ident == Id::dollar;
gen/asm-x86.h:        peekToken()->ident == Id::ptr) {
gen/asm-x86.h:      } else if (token->ident == Id::offset || token->ident == Id::offsetof) {
gen/asm-x86.h:        if (token->ident == Id::offset && !global.params.useDeprecated) {
gen/asm-x86.h:      if (ident == Id::__LOCAL_SIZE) {
gen/asm-x86.h:      if (ident == Id::dollar) {
gen/asm-x86.h:      ident = Id::dollar;
gen/dcompute/druntime.cpp:  if ((*moduleDecl->packages)[0] != Id::ldc)
gen/dcompute/druntime.cpp:  return moduleDecl->id == Id::dcompute;
gen/dcompute/druntime.cpp:  if (sd->ident != Id::dcPointer || !isFromLDC_DCompute(sd))
gen/declarations.cpp:    if (decl->ident == Id::lib) {
gen/function-inlining.cpp:  if (fdecl.getModule()->ident == Id::object) {
gen/functions.cpp:  if (fdecl->ident == Id::ensure || fdecl->ident == Id::require) {
gen/functions.cpp:  assert(fd->ident != Id::empty);
gen/ldctraits.d:    if (e.ident == Id.targetCPU)
gen/ldctraits.d:    if (e.ident == Id.targetHasFeature)
gen/llvmhelpers.cpp:    if (vd->ident == Id::ctfe) {
gen/llvmhelpers.cpp:    if (vd->ident == Id::_arguments && gIR->func()->_arguments) {
gen/llvmhelpers.cpp:      Logger::println("Id::_arguments");
gen/llvmhelpers.cpp:    if (vd->ident == Id::_argptr && gIR->func()->_argptr) {
gen/llvmhelpers.cpp:      Logger::println("Id::_argptr");
gen/llvmhelpers.cpp:    if (vd->ident == Id::dollar) {
gen/llvmhelpers.cpp:      Logger::println("Id::dollar");
gen/pragma.cpp:  if (ident == Id::LDC_intrinsic) {
gen/pragma.cpp:  if (ident == Id::LDC_global_crt_ctor || ident == Id::LDC_global_crt_dtor) {
gen/pragma.cpp:    return ident == Id::LDC_global_crt_ctor ? LLVMglobal_crt_ctor
gen/pragma.cpp:  if (ident == Id::LDC_no_typeinfo) {
gen/pragma.cpp:  if (ident == Id::LDC_no_moduleinfo) {
gen/pragma.cpp:  if (ident == Id::LDC_alloca) {
gen/pragma.cpp:  if (ident == Id::LDC_va_start) {
gen/pragma.cpp:  if (ident == Id::LDC_va_copy) {
gen/pragma.cpp:  if (ident == Id::LDC_va_end) {
gen/pragma.cpp:  if (ident == Id::LDC_va_arg) {
gen/pragma.cpp:  if (ident == Id::LDC_fence) {
gen/pragma.cpp:  if (ident == Id::LDC_atomic_load) {
gen/pragma.cpp:  if (ident == Id::LDC_atomic_store) {
gen/pragma.cpp:  if (ident == Id::LDC_atomic_cmp_xchg) {
gen/pragma.cpp:  if (ident == Id::LDC_atomic_rmw) {
gen/pragma.cpp:  if (ident == Id::LDC_verbose) {
gen/pragma.cpp:  if (ident == Id::LDC_inline_asm) {
gen/pragma.cpp:  if (ident == Id::LDC_inline_ir) {
gen/pragma.cpp:  if (ident == Id::LDC_extern_weak) {
gen/pragma.cpp:  if (ident == Id::LDC_profile_instr) {
gen/statements.cpp:      if (ce->f && ce->f->ident == Id::dcReflect) {
gen/tocall.cpp:    if (dfnval && (dfnval->func->ident == Id::ensure ||
gen/tocall.cpp:                   dfnval->func->ident == Id::require)) {
gen/toir.cpp:    if (ident == Id::ensure || ident == Id::require) {
gen/trycatchfinally.cpp:          irs.ir->CreateICmpEQ(irs.ir->CreateLoad(ehSelectorSlot), ehTypeId),
gen/uda.cpp:      (*moduleDecl->packages)[0] != Id::ldc) {
gen/uda.cpp:  if (isFromMagicModule(sle,Id::attributes)) {
gen/uda.cpp:    if (ident == Id::udaSection) {
gen/uda.cpp:    } else if (ident == Id::udaOptStrategy || ident == Id::udaTarget) {
gen/uda.cpp:    } else if (ident == Id::udaWeak) {
gen/uda.cpp:    if (ident == Id::udaAllocSize) {
gen/uda.cpp:    } else if (ident == Id::udaLLVMAttr) {
gen/uda.cpp:    } else if (ident == Id::udaLLVMFastMathFlag) {
gen/uda.cpp:    } else if (ident == Id::udaOptStrategy) {
gen/uda.cpp:    } else if (ident == Id::udaSection) {
gen/uda.cpp:    } else if (ident == Id::udaTarget) {
gen/uda.cpp:    } else if (ident == Id::udaWeak || ident == Id::udaKernel) {
gen/uda.cpp:  auto sle = getMagicAttribute(sym, Id::udaWeak, Id::attributes);
gen/uda.cpp:  auto sle = getMagicAttribute(sym, Id::udaCompute, Id::dcompute);
gen/uda.cpp:  auto sle = getMagicAttribute(sym, Id::udaKernel, Id::dcompute);

I don't see any issue with using our own cache where required.

[WalterBright]

I don't see any issue with using our own cache where required.

It seems to still be extra work for ldc and gdc. ping @ibuclaw

[jacob-carlborg]

I was asked by @yebblies to do this here #6771 (comment).

[jacob-carlborg]

@klickverbot If that's the whole list, then that's 54. Out of those only 19 exist in the DMD source code.

[dnadlinger]

It seems to still be extra work for ldc and gdc.

It's certainly a very manageable amount of effort. Much less than incorporating the undocumented AST changes occurring from release to release anyway.

[WalterBright]

I still want @ibuclaw 's buyin on this before pulling it.

[dnadlinger]

Reopening as discussion has resumed.

I'm confident that this isn't going to be an issue for GDC either, but while waiting for @ibuclaw's response, we could discuss the actual change at hand. (The implementation seems unnecessarily clunky; see my above review.)

[jacob-carlborg]

Even better than all this: Replace idgen completely by an id!"foo" template that initialises a member variable from IdPool using a static constructor.

@klickverbot is that the best idea, introducing a lost of static constructors. Also that approach would require quite a few other places to be updated to use the template instead.

[ibuclaw]

I'm OK with this in principle. Compilers should really use their own local identifier caches for symbols specific to themselves.

[jacob-carlborg]

So, is there a preference how to do this? The current implementation or what David suggested #6837 (review).

[rainers]

If a C++ header is still needed it could also be generated by some pragma(msg,generate(...)) when compiling id.d with -version=gen_header. The identifiers should remain extern(C++) in this case, though.

+1 for updating the Visual Studio projects, too.

[wilzbach]

FWIW the recent DAutotest was caused by the auto-generated id.d file (details). So I am strongely in favor of getting rid of this auto-generated file - it just tends to make our complex build process more error-prone.

[jacob-carlborg]

Rebased.

[jacob-carlborg]

Why are all tests failing? I don't see the same problem with any other PRs.

[CyberShadow]

Why are all tests failing?

Well, maybe because this PR, as it is now, is introducing a regression? :)

[jacob-carlborg]

Well, maybe because this PR, as it is now, is introducing a regression? :)

I was surprised that it manged to get as far up the chain as to druntime. If anything it would have expected the compiler not to compile.

[joakim-noah]

I'd like this to get in, always good to remove a dependency. I just ran into dealing with this file when cross-compiling ldc for Android/ARM and rather than pull in another dependency- I'd need a D compiler to compile to linux/x64 in addition to the ldc cross-compiler I'm using- I just generated these two files and added them to a patch, seen at termux/termux-packages#1078.

[jacob-carlborg]

The tests are green now. I'm pretty sure that the vibe.d failures are unrelated since other PRs have the same issue..

[wilzbach]

I'm pretty sure that the vibe.d failures are unrelated since other PRs have the same issue..

Yep it's a regression introduced in dlang/phobos#5427

[wilzbach]

Ok, everyone seems to be ok with moving forward, the only open question is:

So, is there a preference how to do this? The current implementation or what David suggested #6837 (review).

[jacob-carlborg]

Can someone please comment on what @klickverbot suggested #6837 (review). I want to know if anyone else agree with what David suggested. I would like to move on with this.

[dnadlinger]

I don't think I can argue against the change as it is, since it doesn't change the interface to Id. it just seemed rather silly to change the code twice.

[yebblies]

The problem with id!"foo" is that it won't protect against typos - which was part of the goal of the current design. This change seems worth pulling with the current design.

[dlang-bot]

Thanks for your pull request, @jacob-carlborg! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[jacob-carlborg]

What's the status of this? Can we get this merged or are any changes required?

No changes required.

[wilzbach]

This change seems worth pulling with the current design.

I agree - this change is immensely useful for build tools down the road. Thanks a lot for pushing this!
I don't feel comfortable yet with signing off non-trivial PRs at DMD, but it seems that we have a consensus for going in this direction. So maybe @andralex or @WalterBright could help this PR to reach the finish line?

[andralex]

@WalterBright and I are in favor of this big time. From the comments our understanding is the gdc and ldc responsibles are on board.

I think there's good value in this suggestion by @klickverbot:

Even better than all this: Replace idgen completely by an id!"foo" template that initialises a member variable from IdPool using a static constructor.

There's liability too, so that work would be exploratory and does not need to come with this PR.

I'll mark this as approved and leave it to @jacob-carlborg to work out the tests. Coverage can be overridden (obviously) but I want to make sure jenkins has no problem.

[wilzbach]

dlang-bot merged commit a3f0ea7 into dlang:master 33 minutes ago

In case someone is wondering, the bot merged this because only DAutoTest and auto-tester are required at the dmd repo. For more details:

dlang/dlang-bot#69

However, the failure at the project tester is an unrelated spurious failure.

[CyberShadow]

This broke dlang.org auto-deploy. I think I know why (Digger uses idgen.d to identify D versions that need a host D compiler), but it shouldn't have passed DAutoTest then. Looking into it :)

[jacob-carlborg]

Thanks.